netdev - Re: [net-next v2 1/8] net-zerocopy: Copy straggler unaligned data for TCP Rx. zerocopy.

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 3 Dec 2020 15:24:53 -0800
From:   Arjun Roy <arjunroy@...gle.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     Stephen Hemminger <stephen@...workplumber.org>,
        Arjun Roy <arjunroy.kdev@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "soheil@...gle.com" <soheil@...gle.com>
Subject: Re: [net-next v2 1/8] net-zerocopy: Copy straggler unaligned data for
 TCP Rx. zerocopy.

On Thu, Dec 3, 2020 at 3:19 PM Arjun Roy <arjunroy@...gle.com> wrote:
>
> On Thu, Dec 3, 2020 at 3:01 PM David Laight <David.Laight@...lab.com> wrote:
> >
> > From: Stephen Hemminger
> > > Sent: 03 December 2020 00:15
> > >
> > > On Wed,  2 Dec 2020 14:09:38 -0800
> > > Arjun Roy <arjunroy.kdev@...il.com> wrote:
> > >
> > > > diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> > > > index cfcb10b75483..62db78b9c1a0 100644
> > > > --- a/include/uapi/linux/tcp.h
> > > > +++ b/include/uapi/linux/tcp.h
> > > > @@ -349,5 +349,7 @@ struct tcp_zerocopy_receive {
> > > >     __u32 recv_skip_hint;   /* out: amount of bytes to skip */
> > > >     __u32 inq; /* out: amount of bytes in read queue */
> > > >     __s32 err; /* out: socket error */
> > > > +   __u64 copybuf_address;  /* in: copybuf address (small reads) */
> > > > +   __s32 copybuf_len; /* in/out: copybuf bytes avail/used or error */
> >
> > You need to swap the order of the above fields to avoid padding
> > and differing alignments for 32bit and 64bit apps.
> >
>
> Just to double check, are you referring to the order of
> copybuf_address and copybuf_len?
> If so, it seems that the current ordering is not creating any
> alignment holes, but flipping it would: https://godbolt.org/z/bdxP6b
>
>
> > > >  };
> > > >  #endif /* _UAPI_LINUX_TCP_H */
> > >
> > > You can't safely grow the size of a userspace API without handling the
> > > case of older applications.  Logic in setsockopt() would have to handle
> > > both old and new sizes of the structure.
> >
> > You also have to allow for old (working) applications being recompiled
> > with the new headers.
> > So you cannot rely on the fields being zero even if you are passed
> > the size of the structure.
> >
>
> I think this should already be taken care of in the current code; the
> full-sized struct with new fields is being zero-initialized, then
> we're getting the user-provided optlen, then copying from userspace
> only that much data. So the newer fields would be zero in that case,
> so this should handle the case of new kernels but old applications.
> Does this address the concern, or am I misunderstanding?
>

Actually, on closer read, perhaps the following is what you have in
mind for the old application?

struct zerocopy_args args;
args.address = ...;
args.length = ...;
args.recv_skip_hint = ...;
args.inq = ...;
args.err = ...;
getsockopt(fd, IPPROTO_TCP, TCP_ZEROCOPY_RECEIVE, &args, sizeof(args));
// sizeof(args) is now bigger when recompiled with new headers, but we
did not explicitly set the new fields to 0, therefore issues

-Arjun

> Thanks,
> -Arjun
>
> >         David
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > Registration No: 1397386 (Wales)
> >