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

[Matthew Dawson <matthew@mjdsystems.ca>]

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

On Friday, August 18, 2017 10:05:18 AM EDT Paolo Abeni wrote:
> On Thu, 2017-08-17 at 22:11 -0400, Matthew Dawson wrote:
> > Due to commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac ("udp: remove
> > headers from UDP packets before queueing"), when udp packets are being
> > peeked the requested extra offset is always 0 as there is no need to skip
> > the udp header.  However, when the offset is 0 and the next skb is
> > of length 0, it is only returned once.  The behaviour can be seen with
> > the following python script:
> > 
> > from socket import *;
> > f=socket(AF_INET6, SOCK_DGRAM | SOCK_NONBLOCK, 0);
> > g=socket(AF_INET6, SOCK_DGRAM | SOCK_NONBLOCK, 0);
> > f.bind(('::', 0));
> > addr=('::1', f.getsockname()[1]);
> > g.sendto(b'', addr)
> > g.sendto(b'b', addr)
> > print(f.recvfrom(10, MSG_PEEK));
> > print(f.recvfrom(10, MSG_PEEK));
> > 
> > Where the expected output should be the empty string twice.
> > 
> > Instead, make sk_peek_offset return negative values, and pass those values
> > to __skb_try_recv_datagram/__skb_try_recv_from_queue.  If the passed
> > offset
> > to __skb_try_recv_from_queue is negative, the checked skb is never
> > skipped.
> > __skb_try_recv_from_queue will then ensure the offset is reset back to 0
> > if a peek is requested without an offset, unless no packets are found.
> > 
> > Also simplify the if condition in __skb_try_recv_from_queue.  If _off is
> > greater then 0, and off is greater then or equal to skb->len, then
> > (_off || skb->len) must always be true assuming skb->len >= 0 is always
> > true.
> > 
> > Also remove a redundant check around a call to sk_peek_offset in
> > af_unix.c,
> > as it double checked if MSG_PEEK was set in the flags.
> > 
> > V2:
> >  - Moved the negative fixup into __skb_try_recv_from_queue, and remove now
> > 
> > redundant checks
> > 
> >  - Fix peeking in udp{,v6}_recvmsg to report the right value when the
> > 
> > offset is 0
> > 
> > Signed-off-by: Matthew Dawson <matthew@mjdsystems.ca>
> > ---
> > 
> >  include/net/sock.h  |  4 +---
> >  net/core/datagram.c | 12 +++++++++---
> >  net/ipv4/udp.c      |  3 ++-
> >  net/ipv6/udp.c      |  3 ++-
> >  net/unix/af_unix.c  |  5 +----
> >  5 files changed, 15 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 7c0632c7e870..aeeec62992ca 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -507,9 +507,7 @@ 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;
> > +		return READ_ONCE(sk->sk_peek_off);
> > 
> >  	}
> >  	
> >  	return 0;
> > 
> > diff --git a/net/core/datagram.c b/net/core/datagram.c
> > index ee5647bd91b3..4b558503bef5 100644
> > --- a/net/core/datagram.c
> > +++ b/net/core/datagram.c
> > @@ -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;
> > +	}
> 
> I think that unlikely() will fit the above condition
Sounds good.

> 
> >  	*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 &&
> > +			    (_off || skb->peeked)) {
> > 
> >  				_off -= skb->len;
> >  				continue;
> >  			
> >  			}
> > 
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index a7c804f73990..cd1d044a7fa5 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -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;
> > 
> > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> > index 578142b7ca3e..20039c8501eb 100644
> > --- a/net/ipv6/udp.c
> > +++ b/net/ipv6/udp.c
> > @@ -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;
> > 
> > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > index 7b52a380d710..be8982b4f8c0 100644
> > --- a/net/unix/af_unix.c
> > +++ b/net/unix/af_unix.c
> > @@ -2304,10 +2304,7 @@ static int unix_stream_read_generic(struct
> > unix_stream_read_state *state,> 
> >  	 */
> >  	
> >  	mutex_lock(&u->iolock);
> > 
> > -	if (flags & MSG_PEEK)
> > -		skip = sk_peek_offset(sk, flags);
> > -	else
> > -		skip = 0;
> > +	skip = max(sk_peek_offset(sk, flags), 0);
> > 
> >  	do {
> >  	
> >  		int chunk;
> 
> later we have:
> 
> 	chunk = min_t(unsigned int, unix_skb_len(skb) - skip, size);
> 
> without any call to __skb_try_recv_from_queue(), so we will get
> bad/unexpected values from the above assignment when 'skip' is
> negative.
The assignment to skip should ensure it is never less then zero, thanks to the 
max(sk...(), 0).  Thus that shouldn't be an issue?

> 
> Overall I still think that adding/using an explicit MSG_PEEK_OFF bit
> would produce a simpler code, but is just a personal preference.
I don't mind either way, that just seemed to be the preference I saw from the 
discussion around the patch.  I think either way will work, so whatever the 
list prefers I'm happy with.

-- 
Matthew

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