Bugzilla – Bug 1319
Ipv6RawSocketImpl IcmpFilter attribute is bugged
Last modified: 2012-01-25 13:20:21 UTC
The attribute is carried in a int32_t, however it should set one bit "up" for each kind of ICMPv6 types. Now, the ICMPv6 types are far more than 32, as type is an 8-bit field, so max value is 255. Now, if we want a bitmask filter, this means that the attribute should be hold in a 255 bit long integer... not viable. Note that for most ICMPv6 messages the type is greater than 128. Hence, the filter will not filter anything. Or better, it will filter everything. The solution is not trivial, as it implies a change in the API and the attribute, that should be a map or a list of types. You could change it in a uint8_t[32] bit it's ugly as hell and setting it would be a nightmare. Note that this bug might as well hold for the corresponding IPv4 Raw Socket. I did not check it but it could. Reference function and lines: bool Ipv6RawSocketImpl::ForwardUp [...] uint8_t type = icmpHeader.GetType (); if ((1 << type) & m_icmpFilter) { /* packet filtered */ return false; } oh, and "(1 << type)" in the above code should be "(uint32_t(1) << type)" anyway, else it will go boom for some values (it's a signed integer, maybe 64bit, maybe 32, maybe 128, but signed). Not a biggie tho, a bug into a bug doesn't mean a bigger bug.
Bump. This bug is a design choice. Either we DO want this feature or we DO NOT CARE about it. In my opinion this feature (i.e., a filter on the ICMPv6 type to be received by the socket) is not that useful and it makes the code quite complex. Moreover, the way to construct such a filter is counterintuitive, as we'd have to switch from an attribute to a list, so requiring a function to access it. Note that the problem is shared by the Ipv4RawSocketImpl, where the issue is present as well. It's just less "noticeable" as ICMPs are not so used in IPv4, plus the used kinds are way less. On the other hand the problem do exist there as well. An user can set a "filter", but the filter is ignored if the ICMP type is greater than 31. Now, my proposal is to avoid user's mistakes by removing altogether this attribute from both. I know that it's quite harsh, but the alternatives are: 1) keep the functionality changing the interface, so removing the attribute and substituting it with a setter/getter function, 2) remove the functionality by dropping the attribute. In either cases there's a change in the interface, so... Opinions ? T.
Does this "filter" thingie mimic some real world socket option? If so, how is that socket option defined in real sockets? They must have had a similar problem?... IMHO, the only reason to have this option in ns-3 is to be consistent with real system socket APIs. But in the real systems, the option is present for performance reasons: copying packets to userspace is expensive, so if we can instruct the kernel to copy just a subset of packets, the performance will increase. In ns-3, it is no problem to let the user socket receive callback do the filtering. But we like that ns-3 is compatible to real sockets, it is useful in many cases.
Ok, after a lot of digging, I found out what I bought to be impossible. The Linux (UNIX?) behavior is "wrong" in the same way as the ns-3 one. So ns-3 is right to be wrong... partially. For IPv4 the filtering is done through a uint32_t, so basically restricting the scope of ICMP filter. Not a biggie (theoretically) as the *used* ICMP kinds are all below 32, even if it IS possible to have types greater than 32. As for IPv6, the filtering is totally different, e.g.: struct icmp6_filter oval; ICMP6_FILTER_SETBLOCKALL (&oval); ICMP6_FILTER_SETPASS (ICMP6_ECHO_REPLY, &oval); setsockopt (icmp6_fd, IPPROTO_ICMPV6, ICMP6_FILTER, &oval, sizeof oval); See also: http://fxr.watson.org/fxr/ident?v=NETBSD3;im=10;i=ICMP6_FILTER_SETBLOCKALL It's for FreeBSD, but it still holds. There are four macros to setup the filter, and the filter itself is: struct icmp6_filter { u_int32_t icmp6_filt[8]; }; So, the alternatives are more than the one originally posted. 1) mimic **exacty** the real world: IPv4 "buggy" (but documented) and IPv6 fixed through some macros/functions. 2) fix both in the "same" way. I'd go for the second, as I don't like obscure caveats on supposedly useful functions. The "macros" can be static functions in the Socket class, with the filter structs being defined there as well, just to be consistent with the interface, or they can be member functions of two new classes: icmpFilter and icmpv6Filter (cleaner) Cheers, Tommaso
(In reply to comment #3) > Ok, after a lot of digging, I found out what I bought to be impossible. > > The Linux (UNIX?) behavior is "wrong" in the same way as the ns-3 one. So ns-3 > is right to be wrong... partially. > > For IPv4 the filtering is done through a uint32_t, so basically restricting the > scope of ICMP filter. Not a biggie (theoretically) as the *used* ICMP kinds are > all below 32, even if it IS possible to have types greater than 32. > > As for IPv6, the filtering is totally different, e.g.: > struct icmp6_filter oval; > ICMP6_FILTER_SETBLOCKALL (&oval); > ICMP6_FILTER_SETPASS (ICMP6_ECHO_REPLY, &oval); > setsockopt (icmp6_fd, IPPROTO_ICMPV6, ICMP6_FILTER, &oval, sizeof oval); > See also: > http://fxr.watson.org/fxr/ident?v=NETBSD3;im=10;i=ICMP6_FILTER_SETBLOCKALL > It's for FreeBSD, but it still holds. > > There are four macros to setup the filter, and the filter itself is: > struct icmp6_filter { > u_int32_t icmp6_filt[8]; > }; > > So, the alternatives are more than the one originally posted. > > 1) mimic **exacty** the real world: IPv4 "buggy" (but documented) and IPv6 > fixed through some macros/functions. > 2) fix both in the "same" way. or 3) leave IPv4 alone (limitations and all) and delete the IPv6 attribute or 4) delete both As Gustavo said, this is not a userspace/kernel performance issue. I don't have a strong opinion about the choices. If someone wants to go to the work of providing this cleanly, I wouldn't object, but I don't feel like it matters that much one way or the other, provided that Ipv6 is either removed or fixed through new methods.
It's not that hard, it's just a matter of defining a couple of new classes, their methods and change the attribute so to use the new class instead of an uint32. I'll do it, even tho I can't promise to do it in one week. I just noticed a huge problem in my 6LoWPAN implementation and I'll have to basically refactor almost everything... sigh. Tommaso
Created attachment 1308 [details] ICMPv6 filters made right This patch does mimic exactly the FreeBSD API (1minute in silence to honor itojun's memory). The Attribute is gone, and being replaced by some handy member functions to block all, block one icmp type, pass all, pass one icmp type, ask if an icmp type is blocked or passing, etc. Blocking and passing are additive, and the use of one way or the other way around (i.e., pass all and block what you don't want, or do the opposite and block all and then set what you want to pass through) is just a user's choice. By default no filter is set.
Created attachment 1309 [details] Ipv4 Raw Socket ICMP filter fixes (docs only) This patch fixes the corresponding bug in IPv4 Raw Sockets. Also in this case, it mimics the UN*X behavior, so the filter is "incomplete". I.e., it works for type < 32. In this case the fix is to point it out in the attribute description and to make sure that the filter mask is uint32_t This, however, makes the two raw sockets (v4 and v6) quite inconsistent. Adding the very same v6 filtering methodology to v4 is trivial (takes 5 minutes).
(In reply to comment #6) > Created attachment 1308 [details] > ICMPv6 filters made right > > This patch does mimic exactly the FreeBSD API (1minute in silence to honor > itojun's memory). > > The Attribute is gone, and being replaced by some handy member functions to > block all, block one icmp type, pass all, pass one icmp type, ask if an icmp > type is blocked or passing, etc. > > Blocking and passing are additive, and the use of one way or the other way > around (i.e., pass all and block what you don't want, or do the opposite and > block all and then set what you want to pass through) is just a user's choice. > By default no filter is set. OK by me.
(In reply to comment #7) > Created attachment 1309 [details] > Ipv4 Raw Socket ICMP filter fixes (docs only) > > This patch fixes the corresponding bug in IPv4 Raw Sockets. > > Also in this case, it mimics the UN*X behavior, so the filter is "incomplete". > I.e., it works for type < 32. > > In this case the fix is to point it out in the attribute description and to > make sure that the filter mask is uint32_t > > This, however, makes the two raw sockets (v4 and v6) quite inconsistent. Adding > the very same v6 filtering methodology to v4 is trivial (takes 5 minutes). I am neutral about the choice. Since you seem to want to make them consistent, I would just comment that I would be OK with that patch too.
Incorrect. For practical uses the icmpv4 as it is is more than enough, as all the normally used ICMPs have "low" types. Fixed with change sets: 7697- 2a250bff1466 7696 - 142e062124f4