Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

[Willem de Bruijn]

On Sun, Mar 6, 2022 at 2:22 PM Samuel Thibault <samuel.thibault@xxxxxxxx> 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.