Hi Pablo, On 12/04/2023 18:04, Pablo Neira Ayuso wrote: > On Wed, Apr 12, 2023 at 05:22:36PM +0200, Matthieu Baerts wrote: >> On 12/04/2023 16:21, Jakub Kicinski wrote: >>> On Thu, 6 Apr 2023 12:45:25 +0200 Matthieu Baerts wrote: (...) >>>> Is it not safer to expose something new to userspace, something >>>> dedicated to what can be visible on the wire? >>>> >>>> Or recommend userspace programs to limit to lower than IPPROTO_RAW >>>> because this number is marked as "reserved" by the IANA anyway [1]? >>>> >>>> Or define something new linked to UINT8_MAX because the layer 4 protocol >>>> field in IP headers is limited to 8 bits? >>>> This limit is not supposed to be directly linked to the one of the enum >>>> you modified. I think we could even say it works "by accident" because >>>> "IPPROTO_RAW" is 255. But OK "IPPROTO_RAW" is there from the "beginning" >>>> [2] :) >>> >>> I'm not an expert but Pablo's patch seems reasonable to me TBH. >>> Maybe I'm missing some extra MPTCP specific context? >> >> I was imagining userspace programs doing something like: >> >> if (protocol < 0 || protocol >= IPPROTO_MAX) >> die(); >> >> syscall(...); > > Is this theoretical, or you think any library might be doing this > already? I lack of sufficient knowledge of the MPTCP ecosystem to > evaluate myself. This is theoretical. But using it with socket's protocol parameter is the only good usage of IPPROTO_MAX for me :-D More seriously, I don't see such things when looking at: https://codesearch.debian.net/search?q=%5CbIPPROTO_MAX%5Cb&literal=0&perpkg=1 IPPROTO_MAX is (re)defined in different libs but not used in many programs, mainly in Netfilter related programs in fact. Even if it is linked to MPTCP, I cannot judge if it can be an issue or not because it depends on how the different libC or other libs/apps are interpreting this IPPROTO_MAX and if they are using it before creating a socket. >> With Pablo's modification and such userspace code, this will break MPTCP >> support. >> >> I'm maybe/probably worry for nothing, I don't know any specific lib >> doing that and to be honest, I don't know what is usually done in libc >> and libs implemented on top of that. On the other hand, it is hard to >> guess how it is used everywhere. >> >> So yes, maybe it is fine? > > It has been 3 years since the update, I think this is the existing > scenario looks like this: > > 1) Some userspace programs that rely on IPPROTO_MAX broke in some way > and people fixed it by using IPPROTO_RAW (as you mentioned Matthieu) > > 2) Some userspace programs rely on the IPPROTO_MAX value in some way and > they are broken (yet they need to be fixed). > > If IPPROTO_MAX is restore, both two type of software described in the > scenarios above will be fine. > > If Matthieu consider that likeliness that MPTCP breaks is low, then I > would go for applying the patch. Even if I continue to think that IPPROTO_MAX should not be used when looking at protocol field visible on the wire, I guess it doesn't make sense for a lib to restrict the socket syscall to < IPPROTO_MAX as well just in case this soft limit is modified later like we did 3 years ago. We didn't have any bug reports saying that it was not possible to create an MPTCP socket because of a lib restricting the protocol field to max 256. In other words, indeed, it looks like the likeliness that MPTCP breaks is low. > Yet another reason: Probably it is also good to restore it to > IPPROTO_MAX so Linux gets aligned again with other unix-like systems > which provide this definition? Some folks might care of portability in > userspace. Good point, I guess we should not have modified IPPROTO_MAX 3 years ago. It looks then OK to apply such patch (with the small fix asked by Jakub). It is just a shame we only see this issue now. Maybe because IPPROTO_MAX is used in such context mainly by Netfilter? :-) Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net
