| Message ID | 20230406092558.459491-1-pablo@netfilter.org |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [net,v2] uapi: linux: restore IPPROTO_MAX to 256 and add IPPROTO_UAPI_MAX | expand |
Hi Pablo, Thank you for sharing the v2 of this patch taking into account MPTCP! On 06/04/2023 11:25, Pablo Neira Ayuso wrote: > IPPROTO_MAX used to be 256, but with the introduction of IPPROTO_MPTCP > definition, IPPROTO_MAX was bumped to 263. > > IPPROTO_MPTCP definition is used for the socket interface from > userspace (ie. uAPI). It is never used in the layer 4 protocol field of > IP headers. (similar to IPPROTO_RAW which is < IPPROTO_MAX) > IPPROTO_* definitions are used anywhere in the kernel as well as in > userspace to set the layer 4 protocol field in IP headers as well as > for uAPI. > > At least in Netfilter, there is code in userspace that relies on > IPPROTO_MAX (not inclusive) to check for the maximum layer 4 protocol. > > This patch restores IPPROTO_MAX to 256 for the maximum protocol number > in the IP headers, and it adds a new IPPROTO_UAPI_MAX for the maximum > protocol number for uAPI. > > Update kernel code to use IPPROTO_UAPI_MAX for inet_diag (mptcp > registers one for this) and the inet{4,6}_create() IP socket API. The modification in the kernel looks good to me. But I don't know how to make sure this will not have any impact on MPTCP on the userspace side, e.g. somewhere before calling the socket syscall, a check could be done to restrict the protocol number to IPPROTO_MAX and then breaking MPTCP support. 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] :) WDYT? Cheers, Matt [1] https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml [2] https://github.com/schwabe/davej-history/blob/9cb9f18b5d26bf176e13edbc0c248d121217c6b3/include/linux/in.h
On Thu, 6 Apr 2023 11:25:58 +0200 Pablo Neira Ayuso wrote: > IPPROTO_RAW = 255, /* Raw IP packets */ > + IPPROTO_MAX, > #define IPPROTO_RAW nit: any reason the IPPROTO_MAX is in between the enum entry and its associated define?
On Thu, 6 Apr 2023 12:45:25 +0200 Matthieu Baerts wrote: > The modification in the kernel looks good to me. But I don't know how to > make sure this will not have any impact on MPTCP on the userspace side, > e.g. somewhere before calling the socket syscall, a check could be done > to restrict the protocol number to IPPROTO_MAX and then breaking MPTCP > support. Then again any code which stores the ipproto in an unsigned char will be broken. A perfect solution is unlikely to exist. > 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?
Hi Jakub, On 12/04/2023 16:21, Jakub Kicinski wrote: > On Thu, 6 Apr 2023 12:45:25 +0200 Matthieu Baerts wrote: >> The modification in the kernel looks good to me. But I don't know how to >> make sure this will not have any impact on MPTCP on the userspace side, >> e.g. somewhere before calling the socket syscall, a check could be done >> to restrict the protocol number to IPPROTO_MAX and then breaking MPTCP >> support. > > Then again any code which stores the ipproto in an unsigned char will > be broken. A perfect solution is unlikely to exist. I wonder if the root cause is not the fact we mix the usage of the protocol parameter from the socket syscall (int/s32) and the protocol field from the IP header (u8). To me, the enum is linked to the socket syscall, not the IP header. In this case, it would make sense to have a dedicated "MAX" macro for the IP header, no? >> 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(...); 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? Cheers, Matt
On Wed, Apr 12, 2023 at 05:22:36PM +0200, Matthieu Baerts wrote: > Hi Jakub, > > On 12/04/2023 16:21, Jakub Kicinski wrote: > > On Thu, 6 Apr 2023 12:45:25 +0200 Matthieu Baerts wrote: > >> The modification in the kernel looks good to me. But I don't know how to > >> make sure this will not have any impact on MPTCP on the userspace side, > >> e.g. somewhere before calling the socket syscall, a check could be done > >> to restrict the protocol number to IPPROTO_MAX and then breaking MPTCP > >> support. > > > > Then again any code which stores the ipproto in an unsigned char will > > be broken. A perfect solution is unlikely to exist. Yes, this is tricky. > I wonder if the root cause is not the fact we mix the usage of the > protocol parameter from the socket syscall (int/s32) and the protocol > field from the IP header (u8). > > To me, the enum is linked to the socket syscall, not the IP header. In > this case, it would make sense to have a dedicated "MAX" macro for the > IP header, no? > > >> 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. > 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. 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.
On Wednesday 2023-04-12 17:22, Matthieu Baerts wrote: >>> But I don't know how to >>> make sure this will not have any impact on MPTCP on the userspace side, >>> e.g. somewhere before calling the socket syscall, a check could be done >>> to restrict the protocol number to IPPROTO_MAX and then breaking MPTCP >>> support. >> >> Then again any code which stores the ipproto in an unsigned char will >> be broken. A perfect solution is unlikely to exist. IPPROTO_MPTCP (262) is new, anything using MPTCP is practically new code for the purposes of discussion, and when MPTCP support is added to some application, you simply *have to* update any potential uint8 check. >I wonder if the root cause is not the fact we mix the usage of the >protocol parameter from the socket syscall (int/s32) and the protocol >field from the IP header (u8). > >To me, the enum is linked to the socket syscall, not the IP header. In >this case, it would make sense to have a dedicated "MAX" macro for the >IP header, no? IPPROTO_MAX (256) was the sensible maximum value [array size] for both the IP header *and* the socket interface. Then the socket interface was extended, so IPPROTO_MAX, at the very least, keeps the meanings it can keep, which is for the one for the IP header. Makes me wonder why MPTCP got 262 instead of just 257.
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
On 12/04/2023 18:14, Jan Engelhardt wrote: > > On Wednesday 2023-04-12 17:22, Matthieu Baerts wrote: >>>> But I don't know how to >>>> make sure this will not have any impact on MPTCP on the userspace side, >>>> e.g. somewhere before calling the socket syscall, a check could be done >>>> to restrict the protocol number to IPPROTO_MAX and then breaking MPTCP >>>> support. >>> >>> Then again any code which stores the ipproto in an unsigned char will >>> be broken. A perfect solution is unlikely to exist. > > IPPROTO_MPTCP (262) is new, anything using MPTCP is practically new code > for the purposes of discussion, and when MPTCP support is added to some > application, you simply *have to* update any potential uint8 check. I hope such check doesn't exist :) IPPROTO_MPTCP is only used when creating the socket, with the "protocol" parameter which accepts an integer. >> I wonder if the root cause is not the fact we mix the usage of the >> protocol parameter from the socket syscall (int/s32) and the protocol >> field from the IP header (u8). >> >> To me, the enum is linked to the socket syscall, not the IP header. In >> this case, it would make sense to have a dedicated "MAX" macro for the >> IP header, no? > > IPPROTO_MAX (256) was the sensible maximum value [array size] > for both the IP header *and* the socket interface. > > Then the socket interface was extended, so IPPROTO_MAX, at the very > least, keeps the meanings it can keep, which is for the one for the > IP header. > Makes me wonder why MPTCP got 262 instead of just 257. Just in case a uint8 is used somewhere, we fallback to TCP (6): IPPROTO_MPTCP & 0xff = IPPROTO_TCP Instead of IPPROTO_ICMP (1). We did that to be on the safe side, not knowing all the different userspace implementations :) Cheers, Matt
On Wednesday 2023-04-12 18:44, Matthieu Baerts wrote: > >> Makes me wonder why MPTCP got 262 instead of just 257. > >Just in case a uint8 is used somewhere, we fallback to TCP (6): > > IPPROTO_MPTCP & 0xff = IPPROTO_TCP > >Instead of IPPROTO_ICMP (1). > >We did that to be on the safe side, not knowing all the different >userspace implementations :) Silent failure? That's terrible. int IPPROTO_MPTCP = 257; socket(AF_INET, SOCK_STREAM, (uint8_t)IPPROTO_MPTCP); on the other hand would immediately fail with EPROTONOSUPP and make hidden uint8 truncation readily visible.
On Wed, 12 Apr 2023 18:35:40 +0200 Matthieu Baerts wrote: > > 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 Perhaps. No strong preference from me. That said I think I can come up with a good name for the SO use: SO_IPPROTO_MAX (which IMHO it's better than IPPROTO_UAPI_MAX if Pablo doesn't mind sed'ing?) The name for a max in proto sense... I'm not sure what that would be. IPPROTO_MAX_IPPROTO ? IP_IPROTO_MAX ? IP_PROTO_MAX ? Dunno.. > 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.
On Wed, Apr 12, 2023 at 12:37:18PM -0700, Jakub Kicinski wrote: > On Wed, 12 Apr 2023 18:35:40 +0200 Matthieu Baerts wrote: > > > 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 > > Perhaps. No strong preference from me. That said I think I can come up > with a good name for the SO use: SO_IPPROTO_MAX (which IMHO it's better > than IPPROTO_UAPI_MAX if Pablo doesn't mind sed'ing?) SO_ is usually reserved for socket options. > The name for a max in proto sense... I'm not sure what that would be. > IPPROTO_MAX_IPPROTO ? IP_IPROTO_MAX ? IP_PROTO_MAX ? Dunno..
diff --git a/include/uapi/linux/in.h b/include/uapi/linux/in.h index 4b7f2df66b99..58ca09438bf0 100644 --- a/include/uapi/linux/in.h +++ b/include/uapi/linux/in.h @@ -80,10 +80,11 @@ enum { IPPROTO_ETHERNET = 143, /* Ethernet-within-IPv6 Encapsulation */ #define IPPROTO_ETHERNET IPPROTO_ETHERNET IPPROTO_RAW = 255, /* Raw IP packets */ + IPPROTO_MAX, #define IPPROTO_RAW IPPROTO_RAW IPPROTO_MPTCP = 262, /* Multipath TCP connection */ #define IPPROTO_MPTCP IPPROTO_MPTCP - IPPROTO_MAX + IPPROTO_UAPI_MAX }; #endif diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 8db6747f892f..4a9061da8983 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -252,7 +252,7 @@ static int inet_create(struct net *net, struct socket *sock, int protocol, int try_loading_module = 0; int err; - if (protocol < 0 || protocol >= IPPROTO_MAX) + if (protocol < 0 || protocol >= IPPROTO_UAPI_MAX) return -EINVAL; sock->state = SS_UNCONNECTED; diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c index b812eb36f0e3..0723bef44827 100644 --- a/net/ipv4/inet_diag.c +++ b/net/ipv4/inet_diag.c @@ -52,7 +52,7 @@ static DEFINE_MUTEX(inet_diag_table_mutex); static const struct inet_diag_handler *inet_diag_lock_handler(int proto) { - if (proto < 0 || proto >= IPPROTO_MAX) { + if (proto < 0 || proto >= IPPROTO_UAPI_MAX) { mutex_lock(&inet_diag_table_mutex); return ERR_PTR(-ENOENT); } @@ -1413,7 +1413,7 @@ int inet_diag_register(const struct inet_diag_handler *h) const __u16 type = h->idiag_type; int err = -EINVAL; - if (type >= IPPROTO_MAX) + if (type >= IPPROTO_UAPI_MAX) goto out; mutex_lock(&inet_diag_table_mutex); @@ -1432,7 +1432,7 @@ void inet_diag_unregister(const struct inet_diag_handler *h) { const __u16 type = h->idiag_type; - if (type >= IPPROTO_MAX) + if (type >= IPPROTO_UAPI_MAX) return; mutex_lock(&inet_diag_table_mutex); @@ -1443,7 +1443,7 @@ EXPORT_SYMBOL_GPL(inet_diag_unregister); static int __init inet_diag_init(void) { - const int inet_diag_table_size = (IPPROTO_MAX * + const int inet_diag_table_size = (IPPROTO_UAPI_MAX * sizeof(struct inet_diag_handler *)); int err = -ENOMEM; diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c index 38689bedfce7..64d5c803f070 100644 --- a/net/ipv6/af_inet6.c +++ b/net/ipv6/af_inet6.c @@ -128,7 +128,7 @@ static int inet6_create(struct net *net, struct socket *sock, int protocol, int try_loading_module = 0; int err; - if (protocol < 0 || protocol >= IPPROTO_MAX) + if (protocol < 0 || protocol >= IPPROTO_UAPI_MAX) return -EINVAL; /* Look for the requested type/protocol pair. */
IPPROTO_MAX used to be 256, but with the introduction of IPPROTO_MPTCP definition, IPPROTO_MAX was bumped to 263. IPPROTO_MPTCP definition is used for the socket interface from userspace (ie. uAPI). It is never used in the layer 4 protocol field of IP headers. IPPROTO_* definitions are used anywhere in the kernel as well as in userspace to set the layer 4 protocol field in IP headers as well as for uAPI. At least in Netfilter, there is code in userspace that relies on IPPROTO_MAX (not inclusive) to check for the maximum layer 4 protocol. This patch restores IPPROTO_MAX to 256 for the maximum protocol number in the IP headers, and it adds a new IPPROTO_UAPI_MAX for the maximum protocol number for uAPI. Update kernel code to use IPPROTO_UAPI_MAX for inet_diag (mptcp registers one for this) and the inet{4,6}_create() IP socket API. Fixes: faf391c3826c ("tcp: Define IPPROTO_MPTCP") Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- v2: Add IPPROTO_UAPI_MAX so mptcp socket API does not break. include/uapi/linux/in.h | 3 ++- net/ipv4/af_inet.c | 2 +- net/ipv4/inet_diag.c | 8 ++++---- net/ipv6/af_inet6.c | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-)