netdev - Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 7 Mar 2022 11:03:56 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Samuel Thibault <samuel.thibault@...ri.fr>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        willemb@...gle.com, davem@...emloft.net, kuba@...nel.org,
        linux-kernel@...r.kernel.org,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT

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