Date:   Tue, 9 Feb 2021 15:46:50 -0800
From:   Arjun Roy <arjunroy@...gle.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     David Ahern <dsahern@...il.com>, Leon Romanovsky <leon@...nel.org>,
        Arjun Roy <arjunroy.kdev@...il.com>,
        David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Soheil Hassas Yeganeh <soheil@...gle.com>
Subject: Re: [net-next v2] tcp: Explicitly mark reserved field in
 tcp_zerocopy_receive args.

On Tue, Feb 9, 2021 at 8:59 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Mon, 8 Feb 2021 20:20:29 -0700 David Ahern wrote:
> > On 2/8/21 7:53 PM, Jakub Kicinski wrote:
> > > On Mon, 8 Feb 2021 19:24:05 -0700 David Ahern wrote:
> > >> That would be the case for new userspace on old kernel. Extending the
> > >> check to the end of the struct would guarantee new userspace can not ask
> > >> for something that the running kernel does not understand.
> > >
> > > Indeed, so we're agreeing that check_zeroed_user() is needed before
> > > original optlen from user space gets truncated?
> >
> > I thought so, but maybe not. To think through this ...
> >
> > If current kernel understands a struct of size N, it can only copy that
> > amount from user to kernel. Anything beyond is ignored in these
> > multiplexed uAPIs, and that is where the new userspace on old kernel falls.
> >
> > Known value checks can only be done up to size N. In this case, the
> > reserved field is at the end of the known struct size, so checking just
> > the field is fine. Going beyond the reserved field has implications for
> > extensions to the API which should be handled when those extensions are
> > added.
>
> Let me try one last time.
>
> There is no check in the kernels that len <= N. User can pass any
> length _already_. check_zeroed_user() forces the values beyond the
> structure length to be known (0) rather than anything. It can only
> avoid breakages in the future.
>
> > So, in short I think the "if (zc.reserved)" is correct as Leon noted.
>
> If it's correct to check some arbitrary part of the buffer is zeroed
> it should be correct to check the entire tail is zeroed.

So, coming back to the thread, I think the following appears to be the
current thoughts:

1. It is requested that, on the kernel as it stands today, fields
beyond zc.msg_flags (including zc.reserved, the only such field as of
this patch) are zero'd out. So a new userspace asking to do specific
things would fail on this old kernel with EINVAL. Old userspace would
work on old or new kernels. New of course works on new kernels.
2. If it's correct to check some arbitrary field (zc.reserved) to be
0, then it should be fine to check this for all future fields >=
reserved in the struct. So some advanced userspace down the line
doesn't get confused.

Strictly speaking, I'm not convinced this is necessary - eg. 64 bytes
struct right now, suppose userspace of the future gives us 96 bytes of
which the last 32 are non-zero for some feature or the other. We, in
the here and now kernel, truncate that length to 64 (as in we only
copy to kernel those first 64 bytes) and set the returned length to
64. The understanding being, any (future, past or present) userspace
consults the output value; and considers anything byte >= the returned
len to be untouched by the kernel executing the call (ie. garbage,
unacted upon).

So, how would this work for old+new userspace on old+new kernel?

A) old+old, new+new: sizes match, no issue
B) new kernel, old userspace: That's not an issue. We have the
switch(len) statement for that.
C) old kernel, new userspace: that's the 96 vs. 64 B example above -
new userspace would see that the kernel only operated on 64 B and
treat the last 32 B as garbage/unacted on.

In this case, we would not give EINVAL on case C, as we would if we
returned EINVAL on a check_zeroed_user() case for fields past
zc.reserved. We'd do a zerocopy operating on just the features we know
about, and communicate to the user that we only acted on features up
until this byte offset.

Now, given this is the case, we still have the padding confusion with
zc.reserved and the current struct size, so we have to force it to 0
as we are doing. But I think we don't need to go beyond this so far.

Thus, my personal preference is to not have the check_zeroed_user()
check. But if the consensus demands it, then it's an easy enough fix.
What are your thoughts?

Thanks,
-Arjun

Powered by blists - more mailing lists