Message-ID: <7405c14e-1fbe-c820-c470-36b0a50b4cae@tessares.net>
Date:   Wed, 12 Apr 2023 18:35:40 +0200
From:   Matthieu Baerts <matthieu.baerts@...sares.net>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     Jakub Kicinski <kuba@...nel.org>, netfilter-devel@...r.kernel.org,
        davem@...emloft.net, netdev@...r.kernel.org, pabeni@...hat.com,
        edumazet@...gle.com, mathew.j.martineau@...ux.intel.com,
        mptcp@...ts.linux.dev
Subject: Re: [PATCH net,v2] uapi: linux: restore IPPROTO_MAX to 256 and add
 IPPROTO_UAPI_MAX

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

Powered by blists - more mailing lists