net: added module for filtering packets based on l2 addresses by haukepetersen · Pull Request #7066 · RIOT-OS/RIOT

haukepetersen · 2017-05-16T15:30:34Z

This module allows to filter network packets base on linker layer addresses. It can be used in two configurations, blacklisting or whitelisting packets. To chose a mode, simply include the right module:

USEMODULE=l2filter_blacklist make ...

or

USEMODULE=l2filter_whitelist make ...

The actual filter list are then controlled via the ifconfig shell command:

> ifconfig 7 set l2filter 79:65:08:22:86:93:9d:5a
...
> ifconfig
2017-05-16 16:41:54,720 - INFO #  ifconfig
2017-05-16 16:41:54,724 - INFO # Iface  7   HWaddr: 23:2a  Channel: 26  NID: 0x23
2017-05-16 16:41:54,729 - INFO #            Long HWaddr: 0e:46:6e:50:7d:d4:23:2a 
2017-05-16 16:41:54,732 - INFO #            TX-Power: 0dBm  State: IDLE 
2017-05-16 16:41:54,738 - INFO #            ACK_REQ  AUTOCCA  MTU:1280  HL:64  6LO  RTR  IPHC  
2017-05-16 16:41:54,741 - INFO #            Source address length: 8
2017-05-16 16:41:54,743 - INFO #            Link type: wireless
2017-05-16 16:41:54,749 - INFO #            inet6 addr: ff02::1/128  scope: local [multicast]
2017-05-16 16:41:54,755 - INFO #            inet6 addr: fe80::c46:6e50:7dd4:232a/64  scope: local
2017-05-16 16:41:54,761 - INFO #            inet6 addr: ff02::1:ffd4:232a/128  scope: local [multicast]
2017-05-16 16:41:54,766 - INFO #            inet6 addr: ff02::1a/128  scope: local [multicast]
2017-05-16 16:41:54,767 - INFO #            
2017-05-16 16:41:54,771 - INFO #            Black-listed link layer addresses:
2017-05-16 16:41:54,775 - INFO #              0: 79:65:08:22:86:93:9d:5a
2017-05-16 16:41:54,775 - INFO # 
2017-05-16 16:41:54,778 - INFO #            Statistics for Layer 2
2017-05-16 16:41:54,781 - INFO #             RX packets 10  bytes 394
2017-05-16 16:41:54,786 - INFO #             TX packets 12 (Multicast: 6)  bytes 431
2017-05-16 16:41:54,789 - INFO #             TX succeeded 11 errors 1
2017-05-16 16:41:54,791 - INFO #            Statistics for IPv6
2017-05-16 16:41:54,794 - INFO #             RX packets 10  bytes 554
2017-05-16 16:41:54,799 - INFO #             TX packets 12 (Multicast: 6)  bytes 682
2017-05-16 16:41:54,802 - INFO #             TX succeeded 12 errors 0
2017-05-16 16:41:54,803 - INFO #

So far this module is hooked for all Ethernet and IEEE802.15.4 devices.

Missing/open issues (will get attention in follow-up PRs):

so far addresses can not be removed via ifconfig -> @PeterKietzmann will add this
a way to pass static list during compile time would be nice
currently the netstats module counts packets that are dropped by the filter -> PR will follow soon

miri64 · 2017-05-16T22:28:10Z

sys/include/net/netopt.h

    NETOPT_NUMOF,
+
+    /**
+     * @brief   add/remove an address to the link layer filter list


What are the required types? How are removal and addition distinguished?

"remove" should be removed here as it needs an other netopt parameter. Addition and removal are not distinguished here

expects a pointer to l2filter_t struct onget or to a hardware address (usually uint8_t) on set

PeterKietzmann · 2017-05-17T07:23:26Z

sys/net/link_layer/l2filter/l2filter.c

+    }
+}
+
+int l2filter_add(l2filter_t *list, const void *addr, size_t addr_len)


This function does not check for duplicate entries. Comparing the IPv6 blacklist module they used a bitfield to detect existing entries, for performance reasons. I can add check later, if you don't have time. @haukepetersen, @miri64 what's you opinion. Should we rather

not check for duplicates

iterate through the list (via 'match() function)

add a bitfield as done in IPv6 blacklist

IMO we should give 2. a try.

TBH I don't have a strong opinion on either option. Since this is a user operation 1. is the most efficient if documented well but it would be different to the IPv6 counterpart indeed. Also I'm not sure which implications this would have for removal.

I decided for (1) intentionally. IMHO this feature is basically made for testing and experiments, where in both cases one has pretty good control and knowledge what one is doing. So there is IMHO no need to catch duplicate entries...

PeterKietzmann · 2017-05-17T07:29:01Z

I can confirm both blacklist and whitelist working on native and some boards. IMO this is a very useful tool which I will need in very near future. However, I'm not 100% happy with introducing two new NETOPT types to add and remove (will be done in a follow-up) entries. @miri64 what's your general opinion on this?
BTW: I will approve after minor comments are addressed and there is general consensus about the concept.

PeterKietzmann · 2017-05-17T09:22:13Z

Please have a look at #38

miri64 · 2017-05-17T10:09:27Z

sys/include/net/netopt.h

    NETOPT_NUMOF,
+
+    /**
+     * @brief   add/remove an address to/from the link layer filter list


Isn't NETOPT_L2FILTER_RM for remove now?

miri64 · 2017-05-17T10:10:00Z

sys/include/net/netopt.h

+     * blacklisting and whitelisting.
+     */
+    NETOPT_L2FILTER,
+    NETOPT_L2FILTER_RM, /**< get will always return -ENOTSUP */


Please add some type doc for this netopt, too.

miri64 · 2017-05-17T10:14:41Z

I can confirm both blacklist and whitelist working on native and some boards. IMO this is a very useful tool which I will need in very near future. However, I'm not 100% happy with introducing two new NETOPT types to add and remove (will be done in a follow-up) entries. @miri64 what's your general opinion on this?

I'm not happy either, but for list operations this seems to be the only way at the momement.

smlng

besides other comments, some suggestions to improve documentation

smlng · 2017-05-17T10:19:40Z

sys/include/net/l2filter.h

+ * @param[in] list      pointer to the filter list
+ * @param[in] addr      address to be added to list
+ * @param[in] addr_len  size of @p addr [in byte]
+ *


missing @pres stating the assert conditions (list and addr != NULL, and addr_len < MAX_ADDRLEN.

smlng · 2017-05-17T10:19:59Z

sys/include/net/l2filter.h

+ * @param[in] list      pointer to the filter list
+ * @param[in] addr      address to remove from the list
+ * @param[in] addr_len  length of @p addr [in byte]
+ *


missing @pre, see above

smlng · 2017-05-17T10:20:35Z

sys/include/net/l2filter.h

+ * @param[in] list      list with black-/whitelisted addresses
+ * @param[in] addr      address to check against the entries in @p list
+ * @param[in] addr_len  length of @p addr [in byte]
+ *


dito see above regarding @pre

smlng · 2017-05-17T10:23:40Z

sys/include/net/l2filter.h

+
+/**
+ * @brief   Number of slots in each filter list (filter entries per device)
+ */


inconsistent documentation here outside of #ifndef, above its within - don't [know] whats (more) correct?

I prefer above, because then it is consistent with constructs like this

/** * @brief Doc-brief * @note 1 if @ref sys_example is present, 2 if not. */ #ifdef MODULE_EXAMPLE #define EXAMPLE_VALUE (1) #else #define EXAMPLE_VALUE (2) #endif

(and yes there is a use-case for this construct. E.g. gnrc_netif uses it to calculate the number of required addresses).

smlng · 2017-05-17T10:25:10Z

sys/include/net/l2filter.h

+/**
+ * @brief   Add an entry to a devices filter list
+ *
+ * @param[in] list      pointer to the filter list


this is actually [in|out], isn't it?

It depends on the point of view but IMO its rather an in because you give in a pointer to the list where you want to add the entry.

if it is an in than this could beconst, but I doubt you can add an element then ...

I am with @smlng here. The element that is pointed to by list (aka list[0]) will be modified (probably at the first addition)

yapp, but its [in,out] :-)

smlng · 2017-05-17T10:25:45Z

sys/include/net/l2filter.h

+/**
+ * @brief   Remove an entry from the given filter list
+ *
+ * @param[in] list      pointer to the filter list


same as above: its [in|out] param

miri64 · 2017-05-17T14:29:09Z

sys/include/net/netopt.h

@@ -248,6 +248,16 @@ typedef enum {
     * @note    Interfaces are not meant to respond to that.
     */
    NETOPT_NUMOF,


Uhm.... better put this below the newly defined netopts ;-)

miri64 · 2017-05-17T14:30:40Z

sys/include/net/netopt.h

+     * blacklisting and whitelisting.
+     */
+    NETOPT_L2FILTER,
+    NETOPT_L2FILTER_RM, /**< get will always return -ENOTSUP */


Btw, I think you need to extend _netopt_strmap, too.

PeterKietzmann · 2017-05-18T07:14:31Z

@miri64, @smlng I addressed your comments and PRed against @haukepetersen's branch #39. If it's just about the doc it would be so great if you approve so we can merge soon

cgundogan · 2017-05-18T07:53:51Z

sys/net/link_layer/l2filter/l2filter.c

+        if (list[i].addr_len == 0) {
+            list[i].addr_len = addr_len;
+            memcpy(list[i].addr, addr, addr_len);
+            res = 0;


I would rather break here instead of testing res in the for loop header (all following for loops, too)

makes probably sense. Some old MISRA rules did not allow to use break (changed in newer MISRA rules, though), so I tend to be stuck not to use break when possible... But times change and I will try to adapt :-)

haukepetersen · 2017-05-18T08:26:40Z

Tried to address all comments (@PeterKietzmann thanks for the help!)

added @pres -> is this convention now to do this for every function?
fixed doc for new netopts
use break in the implementation
optimized shell command for add/removing filters

PeterKietzmann · 2017-05-18T09:26:06Z

So I tested both list variants on boards and on native. Everything works as expected and as far as I see it, all comments were addressed. @miri64 , @smlng please approve

smlng · 2017-05-18T10:07:36Z

drivers/include/net/netdev.h

 #include <sys/uio.h>

 #include "net/netstats.h"
+#include "net/l2filter.h"


ifdef, too?

IMHO no need for this -> netstats does also not do this...

I'm also against #ifdef for header includes (unless 100% needed). Just bloats the top part of the code with more lines and the compiler ignores non-implemented header definitions anyway.

yeah should be fine as long there is no global or external variable definition that could result in undefined reference or unused variable error when the module is not used.

hm, thinking about @smlng's comment: including header files, where the implementation is known to be not present, may still lead to name clashes if those functions / variables are too generic in their names and in the worst case to undefined behavior (I assume there is no big #ifdef in the header file itself). IMO it's also better to take off some work for the preprocessor / compiler by hiding those includes, iff its known that they are not needed. But as @miri64 states, it could get ugly and bloated very quickly.

If you are not happy with that, please provide a follow-up for these cases and let's discuss it there.

This is not how we should review our PRs. Rather than trying to get them in quickly, but sloppy, and rely on follow-up PRs, we should aim for our best in the first iteration.
Does it hurt so much to not copy a previously applied style that might be flawed? It's just two lines more for less problems .. Since when are unused include files not considered as bad coding? (;

If a clash occurs with a header from our codebase the header is flawed IMHO, not the consumer of that header (in the same codebase). I would rather take a look into the header and assure the clash does not happen there (e.g. with proper prefixing according to our naming conventions), and not in the consumers of that header. This is how you build a proper API: not putting the need to remember exceptions on the shoulders of the "user". Mind you, that is not true for external headers, but there you still have can name clashes in the linker (see emb6's clist e.g.) but those need to be fixed by other means anyway.

This is not sloppiness, this is moving tangential discussions (that stall the PR's review) to a separate thread BTW ;-).

If a clash occurs with a header from our codebase the header is flawed IMHO, not the consumer of that header (in the same codebase). I would rather take a look into the header and assure the clash does not happen there (e.g. with proper prefixing according to our naming conventions), and not in the consumers of that header.

Prefixing according to our coding conventions surely helps, but does not resolve the problem of clashing with external packages. You yourself gave a good example for what I am talking about.

Nevertheless, the question is not how to design a good API, but whether a header file should be included that is definitly not used. There are no arguments against ifdefing unused header files, besides the bloating. But, IMO, bloating is a good price to pay to reduce

global name space pollution

compilation time

strange behaviours due to compiler hints, like e.g. pragmas

potential clash of pre-processor statements

.. and depending on the order or a misconfigured build system, external software, where we do not have full control (besides of patching), might be affected by above bullet points.

I see that there is not really a point reasoning here as the discussion will spin around forever. I am just surprised that the harms of unused includes are not considered as a bad style and that flaws (IMO) are blindly copied.

@miri64 go ahead and merge.

Alright and thanks!

miri64 · 2017-05-18T11:10:37Z

makefiles/pseudomodules.inc.mk

 PSEUDOMODULES += gnrc_sixlowpan_router_default
 PSEUDOMODULES += gnrc_sock_check_reuse
 PSEUDOMODULES += gnrc_txtsnd
+PSEUDOMODULES += l2filter_%


So... l2filter_foobar gives me blacklist... Can you give explicit names here. Just having this translate to anything doesn't seem like a good idea. Pseudo-modules are already in-transparent as they are.

yes, will do

miri64 · 2017-05-18T11:13:20Z

sys/shell/commands/sc_netif.c

         "       * \"cca_threshold\" - set ED threshold during CCA in dBm\n"
+#ifdef MODULE_L2FILTER
+         "       * \"l2filter\" - add link layer address to filter\n"
+         "       * \"l2filter_rm\" - remove link layer address from filter\n"


Can the command be ifconfig l2filter add or ifconfig l2filter del please? IP addresses are also lists of addresses and are configured that way.

will also do

miri64 · 2017-05-18T11:17:24Z

Tested for native. Apart from command structure and the pseudomodule names I'm okay with this PR

haukepetersen · 2017-05-18T12:50:31Z

all fixed.

haukepetersen · 2017-05-18T13:06:25Z

and one more minor thing that @miri64 pointed out offline.

miri64

ACK. Please squash

haukepetersen · 2017-05-18T13:20:02Z

squashed.

cgundogan · 2017-05-18T13:24:20Z

all fixed.

I know it's nice and less time-consuming to quickly merge open PRs, but how is #7066 (comment) considered as fixed? There was not even a discussion. IMO, unused includes are more harmful to RIOT than some people might think, especially if it's used throughout the code base.

haukepetersen · 2017-05-18T13:34:22Z

@cgundogan: are you satisfied with the answers above? If yes, feel free to press the big green button :-)

miri64 · 2017-05-18T13:35:19Z

all fixed.

I know it's nice and less time-consuming to quickly merge open PRs, but how is #7066 (comment) considered as fixed? There was not even a discussion. IMO, unused includes are more harmful to RIOT than some people might think, especially if it's used throughout the code base.

@cgundogan As stated above that's a broader discussion that should be discussed in its own issue, so if you agree, please hit the merge button :-).

smlng · 2017-05-18T13:49:02Z

@cgundogan: though I brought up the point and started this discussion in the first place, I'm with @miri64 here to solve this in a separate PR. Cause, as mentioned by @haukepetersen its the same for netstats (and maybe other modules, too). So fixing this here, does not resolve it for the rest. Hence, it make sense to create an issue on that matter or provide a PR for further discussion.

haukepetersen added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels May 16, 2017

haukepetersen added this to the Release 2017.07 milestone May 16, 2017

haukepetersen assigned PeterKietzmann May 16, 2017

haukepetersen requested review from PeterKietzmann and miri64 May 16, 2017 15:30

miri64 requested changes May 16, 2017

View reviewed changes

PeterKietzmann reviewed May 17, 2017

View reviewed changes

miri64 requested changes May 17, 2017

View reviewed changes

smlng suggested changes May 17, 2017

View reviewed changes

miri64 requested changes May 17, 2017

View reviewed changes

cgundogan reviewed May 18, 2017

View reviewed changes

smlng reviewed May 18, 2017

View reviewed changes

smlng approved these changes May 18, 2017

View reviewed changes

miri64 requested changes May 18, 2017

View reviewed changes

PeterKietzmann removed their assignment May 18, 2017

miri64 approved these changes May 18, 2017

View reviewed changes

haukepetersen force-pushed the add_net_l2filter branch from 4fd691c to b45339c Compare May 18, 2017 13:19

haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels May 18, 2017

haukepetersen added 6 commits May 18, 2017 15:24

net: added link layer address filter module

fdadfe2

drivers/netdev: add filter list to netdev strcut

caf97d5

net: added L2FILTER option to netopt

e8b0e0b

net/gnrc/802154: enable filtering of L2 addresses

145e476

net/gnrc/eth: enable filtering of L2 addresses

31dfe38

sys/shell/netif: enable setting of L2 addr filters

359a89f

haukepetersen force-pushed the add_net_l2filter branch from b45339c to 359a89f Compare May 18, 2017 13:24

miri64 merged commit 9ab62b9 into RIOT-OS:master May 18, 2017

haukepetersen deleted the add_net_l2filter branch May 18, 2017 15:00

haukepetersen mentioned this pull request May 18, 2017

sys/drivers: guard l2filter.h, netstats.h includes #7079

Merged

Conversation

haukepetersen commented May 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented May 17, 2017

Uh oh!

PeterKietzmann commented May 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented May 17, 2017

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented May 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented May 18, 2017

Uh oh!

PeterKietzmann commented May 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann May 17, 2017 •

edited

Loading

smlng May 17, 2017 •

edited

Loading