Re: [PATCH net] datagram: When peeking datagrams with offset < 0 don't skip empty skbs

[Matthew Dawson <matthew@mjdsystems.ca>]

[-- Attachment #1: Type: text/plain, Size: 4304 bytes --]

On Wednesday, August 16, 2017 7:27:17 PM EDT Willem de Bruijn wrote:
> On Wed, Aug 16, 2017 at 4:20 PM, Paolo Abeni <pabeni@redhat.com> wrote:
> > On Wed, 2017-08-16 at 11:18 -0400, Willem de Bruijn wrote:
> >> > If I read the above correctly, you are arguining in favor of the
> >> > addittional flag version, right?
> >> 
> >> I was. Though if we are going to thread the argument from the caller
> >> to __skb_try_recv_from_queue to avoid rereading sk->sk_peek_off,
> > 
> >> on second thought it might be simpler to do it through off:
> > [...]
> > 
> >> This, of course, requires restricting sk_peek_off to protect against
> >> overflow.> 
> > Ok, even if I'm not 100% sure overall this will be simpler when adding
> > also the overflow check.
> 
> Actually, it is safe even without the check. Overflow of the signed integer
> is benign here.
> 
> >> If I'm not mistaken, the test in udp_recvmsg currently incorrectly sets
> >> 
> >> peeking to false when peeking at offset zero:
> >>         peeking = off = sk_peek_offset(sk, flags);
> > 
> > I think you are right, does not look correct.
> 
> By shifting the offset by two, we could even make both assignments
> become correct. Return 0 without peek, 1 on peek without SO_PEEK_OFF,
> 2+ otherwise, including overflow up to INT_MIN + 1.
> 
> But the end result is more readable if we just separate those two
> assignments.
> 
> @@ -1574,7 +1574,8 @@ int udp_recvmsg(struct sock *sk, struct msghdr
> *msg, size_t len, int noblock,
>                 return ip_recv_error(sk, msg, len, addr_len);
> 
>  try_again:
> -       peeking = off = sk_peek_offset(sk, flags);
> +       peeking = flags & MSG_PEEK;
> +       off = sk_peek_offset(sk, flags);
>         skb = __skb_recv_udp(sk, flags, noblock, &peeked, &off, &err);
>         if (!skb)
>                 return err;
> 
> @@ -362,7 +362,8 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr
> *msg, size_t len,
>                 return ipv6_recv_rxpmtu(sk, msg, len, addr_len);
> 
>  try_again:
> -       peeking = off = sk_peek_offset(sk, flags);
> +       peeking = flags & MSG_PEEK;
> +       off = sk_peek_offset(sk, flags);
>         skb = __skb_recv_udp(sk, flags, noblock, &peeked, &off, &err);
>         if (!skb)
>                 return err;
> 
> At which point there is also no longer a need for the variable shift
> at sk_peek_offset. Just pass the raw value down to
> __skb_try_recv_from_queue and disambiguate there:
> 
> @@ -506,11 +506,8 @@ int sk_set_peek_off(struct sock *sk, int val);
> 
>  static inline int sk_peek_offset(struct sock *sk, int flags)
>  {
> -       if (unlikely(flags & MSG_PEEK)) {
> -               s32 off = READ_ONCE(sk->sk_peek_off);
> -               if (off >= 0)
> -                       return off;
> -       }
> +       if (unlikely(flags & MSG_PEEK))
> +               return READ_ONCE(sk->sk_peek_off);
> 
>         return 0;
>  }
> 
> @@ -169,14 +169,20 @@ struct sk_buff *__skb_try_recv_from_queue(struct sock
> *sk, int *peeked, int *off, int *err, struct sk_buff **last)
>  {
> +       bool peek_at_off = false;
>         struct sk_buff *skb;
> -       int _off = *off;
> +       int _off = 0;
> +
> +       if (flags & MSG_PEEK && (*off) >= 0) {
> +               peek_at_off = true;
> +               _off = *off;
> +       }
> 
>         *last = queue->prev;
>         skb_queue_walk(queue, skb) {
>                 if (flags & MSG_PEEK) {
> -                       if (_off >= skb->len && (skb->len || _off ||
> -                                                skb->peeked)) {
> +                       if (peek_at_off && _off >= skb->len &&
> +                           (skb->len || _off || skb->peeked)) {
                                ^ I'm pretty sure we can remove this check 
(that skb->len is not zero) in this if statement.  If _off is zero, then skb-
>len must also be zero (since _off >= skb->len, if _off is 0, skb->len <= 0.  
If skb->len can't be negative, then skb->len <= 0 => skb->len == 0).  If _off 
is not zero, then checking skb->len is redundant.
>                                 _off -= skb->len;
>                                 continue;
>                         }
Is this queued to go in already?  Or can I help by updating my patch with what 
was discussed here?  I can do that today if wanted.

-- 
Matthew

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]