diff mbox series

[net-next] tcp-zerocopy: Update returned getsockopt() optlen.

Message ID 20200225060620.76486-1-arjunroy.kdev@gmail.com
State Superseded
Delegated to: David Miller
Headers show
Series [net-next] tcp-zerocopy: Update returned getsockopt() optlen. | expand

Commit Message

Arjun Roy Feb. 25, 2020, 6:06 a.m. UTC 
From: Arjun Roy <arjunroy@google.com>

TCP receive zerocopy currently does not update the returned optlen for
getsockopt(). Thus, userspace cannot properly determine if all the
fields are set in the passed-in struct. This patch sets the optlen
before return, in keeping with the expected operation of getsockopt().

Signed-off-by: Arjun Roy <arjunroy@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
Fixes: c8856c051454 ("tcp-zerocopy: Return inq along with tcp receive
zerocopy.")


---
 net/ipv4/tcp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Eric Dumazet Feb. 25, 2020, 6:28 a.m. UTC | #1 
On Mon, Feb 24, 2020 at 10:06 PM Arjun Roy <arjunroy.kdev@gmail.com> wrote:
>
> From: Arjun Roy <arjunroy@google.com>
>
> TCP receive zerocopy currently does not update the returned optlen for
> getsockopt(). Thus, userspace cannot properly determine if all the
> fields are set in the passed-in struct. This patch sets the optlen
> before return, in keeping with the expected operation of getsockopt().
>
> Signed-off-by: Arjun Roy <arjunroy@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> Fixes: c8856c051454 ("tcp-zerocopy: Return inq along with tcp receive
> zerocopy")


OK, please note for next time :

Fixes: tag should not wrap : It should be a single line.
Preferably it should be the first tag (before your Sob)

Add v2 as in [PATCH v2 net-next]  :  so that reviewers can easily see
which version is the more recent one.


>
> +               if (!err) {
> +                       if (put_user(len, optlen))
> +                               return -EFAULT;

Sorry for not asking this before during our internal review :

Is the cost of the extra STAC / CLAC (on x86) being high enough that it is worth
trying to call put_user() only if user provided a different length ?
Arjun Roy Feb. 25, 2020, 4:48 p.m. UTC | #2 
On Mon, Feb 24, 2020 at 10:28 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, Feb 24, 2020 at 10:06 PM Arjun Roy <arjunroy.kdev@gmail.com> wrote:
> >
> > From: Arjun Roy <arjunroy@google.com>
> >
> > TCP receive zerocopy currently does not update the returned optlen for
> > getsockopt(). Thus, userspace cannot properly determine if all the
> > fields are set in the passed-in struct. This patch sets the optlen
> > before return, in keeping with the expected operation of getsockopt().
> >
> > Signed-off-by: Arjun Roy <arjunroy@google.com>
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > Fixes: c8856c051454 ("tcp-zerocopy: Return inq along with tcp receive
> > zerocopy")
>
>
> OK, please note for next time :
>
> Fixes: tag should not wrap : It should be a single line.
> Preferably it should be the first tag (before your Sob)
>
> Add v2 as in [PATCH v2 net-next]  :  so that reviewers can easily see
> which version is the more recent one.
>
>
> >
> > +               if (!err) {
> > +                       if (put_user(len, optlen))
> > +                               return -EFAULT;
>
> Sorry for not asking this before during our internal review :
>
> Is the cost of the extra STAC / CLAC (on x86) being high enough that it is worth
> trying to call put_user() only if user provided a different length ?

I'll have to defer to someone with more understanding of the overheads
involved in this case.

-Arjun
Arjun Roy Feb. 25, 2020, 5:04 p.m. UTC | #3 
On Tue, Feb 25, 2020 at 8:48 AM Arjun Roy <arjunroy@google.com> wrote:
>
> On Mon, Feb 24, 2020 at 10:28 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Mon, Feb 24, 2020 at 10:06 PM Arjun Roy <arjunroy.kdev@gmail.com> wrote:
> > >
> > > From: Arjun Roy <arjunroy@google.com>
> > >
> > > TCP receive zerocopy currently does not update the returned optlen for
> > > getsockopt(). Thus, userspace cannot properly determine if all the
> > > fields are set in the passed-in struct. This patch sets the optlen
> > > before return, in keeping with the expected operation of getsockopt().
> > >
> > > Signed-off-by: Arjun Roy <arjunroy@google.com>
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
> > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > Fixes: c8856c051454 ("tcp-zerocopy: Return inq along with tcp receive
> > > zerocopy")
> >
> >
> > OK, please note for next time :
> >
> > Fixes: tag should not wrap : It should be a single line.
> > Preferably it should be the first tag (before your Sob)
> >
> > Add v2 as in [PATCH v2 net-next]  :  so that reviewers can easily see
> > which version is the more recent one.
> >
> >
> > >
> > > +               if (!err) {
> > > +                       if (put_user(len, optlen))
> > > +                               return -EFAULT;
> >
> > Sorry for not asking this before during our internal review :
> >
> > Is the cost of the extra STAC / CLAC (on x86) being high enough that it is worth
> > trying to call put_user() only if user provided a different length ?
>
> I'll have to defer to someone with more understanding of the overheads
> involved in this case.
>

Actually, now that I think about it, the (hopefully) common case is
indeed that the kernel and userspace agree on the size of the struct,
so I think just having just that one extra branch to check before
issuing a put_user() would be well worth it compared to all the
instructions in put_user(). I'll send a v2 patch with the change.

Thanks,
-Arjun

> -Arjun
Soheil Hassas Yeganeh Feb. 25, 2020, 5:16 p.m. UTC | #4 
On Tue, Feb 25, 2020 at 12:04 PM Arjun Roy <arjunroy@google.com> wrote:
>
> On Tue, Feb 25, 2020 at 8:48 AM Arjun Roy <arjunroy@google.com> wrote:
> >
> > On Mon, Feb 24, 2020 at 10:28 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Mon, Feb 24, 2020 at 10:06 PM Arjun Roy <arjunroy.kdev@gmail.com> wrote:
> > > >
> > > > From: Arjun Roy <arjunroy@google.com>
> > > >
> > > > TCP receive zerocopy currently does not update the returned optlen for
> > > > getsockopt(). Thus, userspace cannot properly determine if all the
> > > > fields are set in the passed-in struct. This patch sets the optlen
> > > > before return, in keeping with the expected operation of getsockopt().
> > > >
> > > > Signed-off-by: Arjun Roy <arjunroy@google.com>
> > > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > > Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
> > > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > > Fixes: c8856c051454 ("tcp-zerocopy: Return inq along with tcp receive
> > > > zerocopy")
> > >
> > >
> > > OK, please note for next time :
> > >
> > > Fixes: tag should not wrap : It should be a single line.
> > > Preferably it should be the first tag (before your Sob)
> > >
> > > Add v2 as in [PATCH v2 net-next]  :  so that reviewers can easily see
> > > which version is the more recent one.
> > >
> > >
> > > >
> > > > +               if (!err) {
> > > > +                       if (put_user(len, optlen))
> > > > +                               return -EFAULT;
> > >
> > > Sorry for not asking this before during our internal review :
> > >
> > > Is the cost of the extra STAC / CLAC (on x86) being high enough that it is worth
> > > trying to call put_user() only if user provided a different length ?
> >
> > I'll have to defer to someone with more understanding of the overheads
> > involved in this case.
> >
>
> Actually, now that I think about it, the (hopefully) common case is
> indeed that the kernel and userspace agree on the size of the struct,
> so I think just having just that one extra branch to check before
> issuing a put_user() would be well worth it compared to all the
> instructions in put_user(). I'll send a v2 patch with the change.

Thank you, Arjun.  Given that most TCP socket options overwrite the
optlen even when returning error, I think we can avoid having the
extra branch by simply moving put_user right after the check for "len
== sizeof(zc)" and before "switch(len)".

Thanks,
Soheil

> Thanks,
> -Arjun
>
> > -Arjun
Arjun Roy Feb. 25, 2020, 8:20 p.m. UTC | #5 
On Tue, Feb 25, 2020 at 9:17 AM Soheil Hassas Yeganeh <soheil@google.com> wrote:
>
> On Tue, Feb 25, 2020 at 12:04 PM Arjun Roy <arjunroy@google.com> wrote:
> >
> > On Tue, Feb 25, 2020 at 8:48 AM Arjun Roy <arjunroy@google.com> wrote:
> > >
> > > On Mon, Feb 24, 2020 at 10:28 PM Eric Dumazet <edumazet@google.com> wrote:
> > > >
> > > > On Mon, Feb 24, 2020 at 10:06 PM Arjun Roy <arjunroy.kdev@gmail.com> wrote:
> > > > >
> > > > > From: Arjun Roy <arjunroy@google.com>
> > > > >
> > > > > TCP receive zerocopy currently does not update the returned optlen for
> > > > > getsockopt(). Thus, userspace cannot properly determine if all the
> > > > > fields are set in the passed-in struct. This patch sets the optlen
> > > > > before return, in keeping with the expected operation of getsockopt().
> > > > >
> > > > > Signed-off-by: Arjun Roy <arjunroy@google.com>
> > > > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > > > Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
> > > > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > > > Fixes: c8856c051454 ("tcp-zerocopy: Return inq along with tcp receive
> > > > > zerocopy")
> > > >
> > > >
> > > > OK, please note for next time :
> > > >
> > > > Fixes: tag should not wrap : It should be a single line.
> > > > Preferably it should be the first tag (before your Sob)
> > > >
> > > > Add v2 as in [PATCH v2 net-next]  :  so that reviewers can easily see
> > > > which version is the more recent one.
> > > >
> > > >
> > > > >
> > > > > +               if (!err) {
> > > > > +                       if (put_user(len, optlen))
> > > > > +                               return -EFAULT;
> > > >
> > > > Sorry for not asking this before during our internal review :
> > > >
> > > > Is the cost of the extra STAC / CLAC (on x86) being high enough that it is worth
> > > > trying to call put_user() only if user provided a different length ?
> > >
> > > I'll have to defer to someone with more understanding of the overheads
> > > involved in this case.
> > >
> >
> > Actually, now that I think about it, the (hopefully) common case is
> > indeed that the kernel and userspace agree on the size of the struct,
> > so I think just having just that one extra branch to check before
> > issuing a put_user() would be well worth it compared to all the
> > instructions in put_user(). I'll send a v2 patch with the change.
>
> Thank you, Arjun.  Given that most TCP socket options overwrite the
> optlen even when returning error, I think we can avoid having the
> extra branch by simply moving put_user right after the check for "len
> == sizeof(zc)" and before "switch(len)".
>
> Thanks,
> Soheil
>
Unfortunately I don't that works in this case - there's a point before
then that could set len to sizeof(zc) (if len was > sizeof(zc) to
begin with) which would disrupt what we want.

Accounting for that would probably add more complication and still
require a branch, so I'm going with the simpler move in this case.
Will send a v2 patch out momentarily.

Thanks,
-Arjun


> > Thanks,
> > -Arjun
> >
> > > -Arjun
diff mbox series

Patch

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 600deb39f17de..fb9894d3d30e9 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4148,8 +4148,12 @@  static int do_tcp_getsockopt(struct sock *sk, int level,
 zerocopy_rcv_inq:
 		zc.inq = tcp_inq_hint(sk);
 zerocopy_rcv_out:
-		if (!err && copy_to_user(optval, &zc, len))
-			err = -EFAULT;
+		if (!err) {
+			if (put_user(len, optlen))
+				return -EFAULT;
+			if (copy_to_user(optval, &zc, len))
+				return -EFAULT;
+		}
 		return err;
 	}
 #endif