[PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

[PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

[Samuel Thibault]

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)

Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

[Willem de Bruijn]

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).

Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

[Samuel Thibault]

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

Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

[Willem de Bruijn]

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.

Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

[Samuel Thibault]

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

Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

[Willem de Bruijn]

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.

Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

[Samuel Thibault]

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

Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

[Samuel Thibault]

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

Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

[Willem de Bruijn]

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.