* Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT [not found] <20220301144453.snstwdjy3kmpi4zf@begin> @ 2022-03-01 14:51 ` Willem de Bruijn 2022-03-01 15:00 ` Samuel Thibault 0 siblings, 1 reply; 8+ messages in thread From: Willem de Bruijn @ 2022-03-01 14:51 UTC (permalink / raw) To: Samuel Thibault, willemb, davem, kuba, linux-kernel; +Cc: Network Development On Tue, Mar 1, 2022 at 9:44 AM Samuel Thibault <samuel.thibault@labri.fr> wrote: > > ENOTSUPP is documented as "should never be seen by user programs", and > is not exposed in <errno.h>, so applications cannot safely check against > it. We should rather return the well-known -ENOPROTOOPT. > > Signed-off-by: Samuel Thibault <samuel.thibault@labri.fr> > > diff --git a/net/core/sock.c b/net/core/sock.c > index 4ff806d71921..6e5b84194d56 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -1377,9 +1377,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname, > if (!(sk_is_tcp(sk) || > (sk->sk_type == SOCK_DGRAM && > sk->sk_protocol == IPPROTO_UDP))) > - ret = -ENOTSUPP; > + ret = -ENOPROTOOPT; > } else if (sk->sk_family != PF_RDS) { > - ret = -ENOTSUPP; > + ret = -ENOPROTOOPT; > } > if (!ret) { > if (val < 0 || val > 1) That should have been a public error code. Perhaps rather EOPNOTSUPP. The problem with a change now is that it will confuse existing applications that check for -524 (ENOTSUPP). ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT 2022-03-01 14:51 ` [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT Willem de Bruijn @ 2022-03-01 15:00 ` Samuel Thibault 2022-03-01 15:14 ` Willem de Bruijn 0 siblings, 1 reply; 8+ messages in thread From: Samuel Thibault @ 2022-03-01 15:00 UTC (permalink / raw) To: Willem de Bruijn; +Cc: willemb, davem, kuba, linux-kernel, Network Development Willem de Bruijn, le mar. 01 mars 2022 09:51:45 -0500, a ecrit: > On Tue, Mar 1, 2022 at 9:44 AM Samuel Thibault <samuel.thibault@labri.fr> wrote: > > > > ENOTSUPP is documented as "should never be seen by user programs", and > > is not exposed in <errno.h>, so applications cannot safely check against > > it. We should rather return the well-known -ENOPROTOOPT. > > > > Signed-off-by: Samuel Thibault <samuel.thibault@labri.fr> > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > index 4ff806d71921..6e5b84194d56 100644 > > --- a/net/core/sock.c > > +++ b/net/core/sock.c > > @@ -1377,9 +1377,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname, > > if (!(sk_is_tcp(sk) || > > (sk->sk_type == SOCK_DGRAM && > > sk->sk_protocol == IPPROTO_UDP))) > > - ret = -ENOTSUPP; > > + ret = -ENOPROTOOPT; > > } else if (sk->sk_family != PF_RDS) { > > - ret = -ENOTSUPP; > > + ret = -ENOPROTOOPT; > > } > > if (!ret) { > > if (val < 0 || val > 1) > > That should have been a public error code. Perhaps rather EOPNOTSUPP. > > The problem with a change now is that it will confuse existing > applications that check for -524 (ENOTSUPP). They were not supposed to hardcord -524... Actually, they already had to check against EOPNOTSUPP to support older kernels, so EOPNOTSUPP is not supposed to pose a problem. Samuel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT 2022-03-01 15:00 ` Samuel Thibault @ 2022-03-01 15:14 ` Willem de Bruijn 2022-03-01 15:20 ` Samuel Thibault 0 siblings, 1 reply; 8+ messages in thread From: Willem de Bruijn @ 2022-03-01 15:14 UTC (permalink / raw) To: Samuel Thibault, Willem de Bruijn, willemb, davem, kuba, linux-kernel, Network Development On Tue, Mar 1, 2022 at 10:00 AM Samuel Thibault <samuel.thibault@labri.fr> wrote: > > Willem de Bruijn, le mar. 01 mars 2022 09:51:45 -0500, a ecrit: > > On Tue, Mar 1, 2022 at 9:44 AM Samuel Thibault <samuel.thibault@labri.fr> wrote: > > > > > > ENOTSUPP is documented as "should never be seen by user programs", and > > > is not exposed in <errno.h>, so applications cannot safely check against > > > it. We should rather return the well-known -ENOPROTOOPT. > > > > > > Signed-off-by: Samuel Thibault <samuel.thibault@labri.fr> > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > > index 4ff806d71921..6e5b84194d56 100644 > > > --- a/net/core/sock.c > > > +++ b/net/core/sock.c > > > @@ -1377,9 +1377,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname, > > > if (!(sk_is_tcp(sk) || > > > (sk->sk_type == SOCK_DGRAM && > > > sk->sk_protocol == IPPROTO_UDP))) > > > - ret = -ENOTSUPP; > > > + ret = -ENOPROTOOPT; > > > } else if (sk->sk_family != PF_RDS) { > > > - ret = -ENOTSUPP; > > > + ret = -ENOPROTOOPT; > > > } > > > if (!ret) { > > > if (val < 0 || val > 1) > > > > That should have been a public error code. Perhaps rather EOPNOTSUPP. > > > > The problem with a change now is that it will confuse existing > > applications that check for -524 (ENOTSUPP). > > They were not supposed to hardcord -524... > > Actually, they already had to check against EOPNOTSUPP to support older > kernels, so EOPNOTSUPP is not supposed to pose a problem. Which older kernels returned EOPNOTSUPP on SO_ZEROCOPY? There is prior art in changing this error code when it leaks to userspace, such as commit 2230a7ef5198 ("drop_monitor: Use correct error code") and commit 4a5cdc604b9c ("net/tls: Fix return values to avoid ENOTSUPP"). I certainly wrote code in the past that explicitly checks for 524 (ENOTSUPP). But do not immediately see public code that does this. Indeed, udpgso_bench_tx checks for both these codes. So it's probably fine. Note that there is some risk. But no more than with those prior commits. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT 2022-03-01 15:14 ` Willem de Bruijn @ 2022-03-01 15:20 ` Samuel Thibault 2022-03-01 15:21 ` Willem de Bruijn 0 siblings, 1 reply; 8+ messages in thread From: Samuel Thibault @ 2022-03-01 15:20 UTC (permalink / raw) To: Willem de Bruijn; +Cc: willemb, davem, kuba, linux-kernel, Network Development Willem de Bruijn, le mar. 01 mars 2022 10:14:18 -0500, a ecrit: > On Tue, Mar 1, 2022 at 10:00 AM Samuel Thibault > <samuel.thibault@labri.fr> wrote: > > > > Willem de Bruijn, le mar. 01 mars 2022 09:51:45 -0500, a ecrit: > > > On Tue, Mar 1, 2022 at 9:44 AM Samuel Thibault <samuel.thibault@labri.fr> wrote: > > > > > > > > ENOTSUPP is documented as "should never be seen by user programs", and > > > > is not exposed in <errno.h>, so applications cannot safely check against > > > > it. We should rather return the well-known -ENOPROTOOPT. > > > > > > > > Signed-off-by: Samuel Thibault <samuel.thibault@labri.fr> > > > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > > > index 4ff806d71921..6e5b84194d56 100644 > > > > --- a/net/core/sock.c > > > > +++ b/net/core/sock.c > > > > @@ -1377,9 +1377,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname, > > > > if (!(sk_is_tcp(sk) || > > > > (sk->sk_type == SOCK_DGRAM && > > > > sk->sk_protocol == IPPROTO_UDP))) > > > > - ret = -ENOTSUPP; > > > > + ret = -ENOPROTOOPT; > > > > } else if (sk->sk_family != PF_RDS) { > > > > - ret = -ENOTSUPP; > > > > + ret = -ENOPROTOOPT; > > > > } > > > > if (!ret) { > > > > if (val < 0 || val > 1) > > > > > > That should have been a public error code. Perhaps rather EOPNOTSUPP. > > > > > > The problem with a change now is that it will confuse existing > > > applications that check for -524 (ENOTSUPP). > > > > They were not supposed to hardcord -524... > > > > Actually, they already had to check against EOPNOTSUPP to support older > > kernels, so EOPNOTSUPP is not supposed to pose a problem. > > Which older kernels returned EOPNOTSUPP on SO_ZEROCOPY? Sorry, bad copy/paste, I meant ENOPROTOOPT. Samuel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT 2022-03-01 15:20 ` Samuel Thibault @ 2022-03-01 15:21 ` Willem de Bruijn 2022-03-01 15:25 ` Samuel Thibault 2022-03-06 19:22 ` Samuel Thibault 0 siblings, 2 replies; 8+ messages in thread From: Willem de Bruijn @ 2022-03-01 15:21 UTC (permalink / raw) To: Samuel Thibault, Willem de Bruijn, willemb, davem, kuba, linux-kernel, Network Development On Tue, Mar 1, 2022 at 10:20 AM Samuel Thibault <samuel.thibault@labri.fr> wrote: > > Willem de Bruijn, le mar. 01 mars 2022 10:14:18 -0500, a ecrit: > > On Tue, Mar 1, 2022 at 10:00 AM Samuel Thibault > > <samuel.thibault@labri.fr> wrote: > > > > > > Willem de Bruijn, le mar. 01 mars 2022 09:51:45 -0500, a ecrit: > > > > On Tue, Mar 1, 2022 at 9:44 AM Samuel Thibault <samuel.thibault@labri.fr> wrote: > > > > > > > > > > ENOTSUPP is documented as "should never be seen by user programs", and > > > > > is not exposed in <errno.h>, so applications cannot safely check against > > > > > it. We should rather return the well-known -ENOPROTOOPT. > > > > > > > > > > Signed-off-by: Samuel Thibault <samuel.thibault@labri.fr> > > > > > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > > > > index 4ff806d71921..6e5b84194d56 100644 > > > > > --- a/net/core/sock.c > > > > > +++ b/net/core/sock.c > > > > > @@ -1377,9 +1377,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname, > > > > > if (!(sk_is_tcp(sk) || > > > > > (sk->sk_type == SOCK_DGRAM && > > > > > sk->sk_protocol == IPPROTO_UDP))) > > > > > - ret = -ENOTSUPP; > > > > > + ret = -ENOPROTOOPT; > > > > > } else if (sk->sk_family != PF_RDS) { > > > > > - ret = -ENOTSUPP; > > > > > + ret = -ENOPROTOOPT; > > > > > } > > > > > if (!ret) { > > > > > if (val < 0 || val > 1) > > > > > > > > That should have been a public error code. Perhaps rather EOPNOTSUPP. > > > > > > > > The problem with a change now is that it will confuse existing > > > > applications that check for -524 (ENOTSUPP). > > > > > > They were not supposed to hardcord -524... > > > > > > Actually, they already had to check against EOPNOTSUPP to support older > > > kernels, so EOPNOTSUPP is not supposed to pose a problem. > > > > Which older kernels returned EOPNOTSUPP on SO_ZEROCOPY? > > Sorry, bad copy/paste, I meant ENOPROTOOPT. Same point though, right? These are not legacy concerns, but specific to applications written to SO_ZEROCOPY. I expect that most will just ignore the exact error code and will work with either. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT 2022-03-01 15:21 ` Willem de Bruijn @ 2022-03-01 15:25 ` Samuel Thibault 2022-03-06 19:22 ` Samuel Thibault 1 sibling, 0 replies; 8+ messages in thread From: Samuel Thibault @ 2022-03-01 15:25 UTC (permalink / raw) To: Willem de Bruijn; +Cc: willemb, davem, kuba, linux-kernel, Network Development Willem de Bruijn, le mar. 01 mars 2022 10:21:41 -0500, a ecrit: > On Tue, Mar 1, 2022 at 10:20 AM Samuel Thibault > <samuel.thibault@labri.fr> wrote: > > > > Willem de Bruijn, le mar. 01 mars 2022 10:14:18 -0500, a ecrit: > > > On Tue, Mar 1, 2022 at 10:00 AM Samuel Thibault > > > <samuel.thibault@labri.fr> wrote: > > > > > > > > Willem de Bruijn, le mar. 01 mars 2022 09:51:45 -0500, a ecrit: > > > > > On Tue, Mar 1, 2022 at 9:44 AM Samuel Thibault <samuel.thibault@labri.fr> wrote: > > > > > > > > > > > > ENOTSUPP is documented as "should never be seen by user programs", and > > > > > > is not exposed in <errno.h>, so applications cannot safely check against > > > > > > it. We should rather return the well-known -ENOPROTOOPT. > > > > > > > > > > > > Signed-off-by: Samuel Thibault <samuel.thibault@labri.fr> > > > > > > > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > > > > > index 4ff806d71921..6e5b84194d56 100644 > > > > > > --- a/net/core/sock.c > > > > > > +++ b/net/core/sock.c > > > > > > @@ -1377,9 +1377,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname, > > > > > > if (!(sk_is_tcp(sk) || > > > > > > (sk->sk_type == SOCK_DGRAM && > > > > > > sk->sk_protocol == IPPROTO_UDP))) > > > > > > - ret = -ENOTSUPP; > > > > > > + ret = -ENOPROTOOPT; > > > > > > } else if (sk->sk_family != PF_RDS) { > > > > > > - ret = -ENOTSUPP; > > > > > > + ret = -ENOPROTOOPT; > > > > > > } > > > > > > if (!ret) { > > > > > > if (val < 0 || val > 1) > > > > > > > > > > That should have been a public error code. Perhaps rather EOPNOTSUPP. > > > > > > > > > > The problem with a change now is that it will confuse existing > > > > > applications that check for -524 (ENOTSUPP). > > > > > > > > They were not supposed to hardcord -524... > > > > > > > > Actually, they already had to check against EOPNOTSUPP to support older > > > > kernels, so EOPNOTSUPP is not supposed to pose a problem. > > > > > > Which older kernels returned EOPNOTSUPP on SO_ZEROCOPY? > > > > Sorry, bad copy/paste, I meant ENOPROTOOPT. > > Same point though, right? These are not legacy concerns, but specific > to applications written to SO_ZEROCOPY. > > I expect that most will just ignore the exact error code and will work > with either. Well, in the code I just wrote, I ignored ENOPROTOOPT due to older kernels, and it's only when the code happened to be run against PF_LOCAL sockets that the ENOTSUPP question raised. My code has never been exposed to a EOPNOTSUPP error, so I would have never written a check against EOPNOTSUPP if you didn't mention it as a possibility. Samuel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT 2022-03-01 15:21 ` Willem de Bruijn 2022-03-01 15:25 ` Samuel Thibault @ 2022-03-06 19:22 ` Samuel Thibault 2022-03-07 16:03 ` Willem de Bruijn 1 sibling, 1 reply; 8+ messages in thread From: Samuel Thibault @ 2022-03-06 19:22 UTC (permalink / raw) To: Willem de Bruijn; +Cc: willemb, davem, kuba, linux-kernel, Network Development Hello, Willem de Bruijn, le mar. 01 mars 2022 10:21:41 -0500, a ecrit: > > > > > > @@ -1377,9 +1377,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname, > > > > > > if (!(sk_is_tcp(sk) || > > > > > > (sk->sk_type == SOCK_DGRAM && > > > > > > sk->sk_protocol == IPPROTO_UDP))) > > > > > > - ret = -ENOTSUPP; > > > > > > + ret = -ENOPROTOOPT; > > > > > > } else if (sk->sk_family != PF_RDS) { > > > > > > - ret = -ENOTSUPP; > > > > > > + ret = -ENOPROTOOPT; > > > > > > } > > > > > > if (!ret) { > > > > > > if (val < 0 || val > 1) > > > > > > > > > > That should have been a public error code. Perhaps rather EOPNOTSUPP. > > > > > > > > > > The problem with a change now is that it will confuse existing > > > > > applications that check for -524 (ENOTSUPP). > > > > > > > > They were not supposed to hardcord -524... > > > > > > > > Actually, they already had to check against EOPNOTSUPP to support older > > > > kernels, so EOPNOTSUPP is not supposed to pose a problem. > > > > > > Which older kernels returned EOPNOTSUPP on SO_ZEROCOPY? > > > > Sorry, bad copy/paste, I meant ENOPROTOOPT. > > Same point though, right? These are not legacy concerns, but specific > to applications written to SO_ZEROCOPY. > > I expect that most will just ignore the exact error code and will work > with either. Ok, so, is this an Acked-by: you? :) Samuel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT 2022-03-06 19:22 ` Samuel Thibault @ 2022-03-07 16:03 ` Willem de Bruijn 0 siblings, 0 replies; 8+ messages in thread From: Willem de Bruijn @ 2022-03-07 16:03 UTC (permalink / raw) To: Samuel Thibault, Willem de Bruijn, willemb, davem, kuba, linux-kernel, Network Development On Sun, Mar 6, 2022 at 2:22 PM Samuel Thibault <samuel.thibault@labri.fr> wrote: > > Hello, > > Willem de Bruijn, le mar. 01 mars 2022 10:21:41 -0500, a ecrit: > > > > > > > @@ -1377,9 +1377,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname, > > > > > > > if (!(sk_is_tcp(sk) || > > > > > > > (sk->sk_type == SOCK_DGRAM && > > > > > > > sk->sk_protocol == IPPROTO_UDP))) > > > > > > > - ret = -ENOTSUPP; > > > > > > > + ret = -ENOPROTOOPT; > > > > > > > } else if (sk->sk_family != PF_RDS) { > > > > > > > - ret = -ENOTSUPP; > > > > > > > + ret = -ENOPROTOOPT; > > > > > > > } > > > > > > > if (!ret) { > > > > > > > if (val < 0 || val > 1) > > > > > > > > > > > > That should have been a public error code. Perhaps rather EOPNOTSUPP. > > > > > > > > > > > > The problem with a change now is that it will confuse existing > > > > > > applications that check for -524 (ENOTSUPP). > > > > > > > > > > They were not supposed to hardcord -524... > > > > > > > > > > Actually, they already had to check against EOPNOTSUPP to support older > > > > > kernels, so EOPNOTSUPP is not supposed to pose a problem. > > > > > > > > Which older kernels returned EOPNOTSUPP on SO_ZEROCOPY? > > > > > > Sorry, bad copy/paste, I meant ENOPROTOOPT. > > > > Same point though, right? These are not legacy concerns, but specific > > to applications written to SO_ZEROCOPY. > > > > I expect that most will just ignore the exact error code and will work > > with either. > > Ok, so, is this an Acked-by: you? :) I did not touch this code on purpose, due to the small risk of legacy users that expect 524. If you think the benefit outweighs the risk --the same conclusion reached in the commits I mentioned, 2230a7ef5198 ("drop_monitor: Use correct error code") and commit 4a5cdc604b9c ("net/tls: Fix return values to avoid ENOTSUPP")-- then I can support that. But like those, I think the correct error code then is EOPNOTSUPP. And the commit message would be stronger by explicitly referencing that prior art, and the fact that those changes did not seem to cause problems. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-03-07 16:04 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220301144453.snstwdjy3kmpi4zf@begin>
2022-03-01 14:51 ` [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT Willem de Bruijn
2022-03-01 15:00 ` Samuel Thibault
2022-03-01 15:14 ` Willem de Bruijn
2022-03-01 15:20 ` Samuel Thibault
2022-03-01 15:21 ` Willem de Bruijn
2022-03-01 15:25 ` Samuel Thibault
2022-03-06 19:22 ` Samuel Thibault
2022-03-07 16:03 ` Willem de Bruijn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox