Re: [PATCH net] datagram: When peeking datagrams with offset < 0 don't skip empty skbs
flat view
From: Paolo Abeni <pabeni@redhat.com>
Date: 2017-08-15 15:40:44
Subsystem:
networking [general], networking [sockets], the rest, user datagram protocol (udp) · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kuniyuki Iwashima, Willem de Bruijn, Linus Torvalds, Willem de Bruijn
On Mon, 2017-08-14 at 21:35 -0400, Willem de Bruijn wrote:
quoted hunk ↗ jump to hunk
It is not infeasible to fix that and fix up all callers, as Matthew's patch does. But perhaps this patch is simpler to reason about. Thoughts? +static inline bool sk_peek_at_offset(struct sock *sk) +{ + return READ_ONCE(sk->sk_peek_off) >= 0; +} + static inline int sk_peek_offset(struct sock *sk, int flags) { if (unlikely(flags & MSG_PEEK)) {diff --git a/net/core/datagram.c b/net/core/datagram.c index ee5647bd91b3..30b53932af73 100644 --- a/net/core/datagram.c +++ b/net/core/datagram.c@@ -175,12 +175,14 @@ struct sk_buff *__skb_try_recv_from_queue(struct sock *sk, *last = queue->prev; skb_queue_walk(queue, skb) { if (flags & MSG_PEEK) { - if (_off >= skb->len && (skb->len || _off || - skb->peeked)) { + if (_off >= skb->len && sk_peek_at_offset(sk) && + (skb->len || _off || skb->peeked)) {
I like the idea a lot, but I think/fear that bad things could happen since sk_peek_off is read twice at different times: one in sk_peek_offset() and one in the above chunk. If the above is not an issue (I am not sure) I'm very fine with this approach. For the record, I thought something like the following (uncomplete, does not even compile): ---
diff --git a/include/linux/socket.h b/include/linux/socket.h
index 8b13db5163cc..5085cf003b88 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h@@ -286,6 +286,7 @@ struct ucred { #define MSG_SENDPAGE_NOTLAST 0x20000 /* sendpage() internal : not the last page */ #define MSG_BATCH 0x40000 /* sendmmsg(): more messages coming */ #define MSG_EOF MSG_FIN +#define MSG_PEEK_OFF 0x80000 #define MSG_FASTOPEN 0x20000000 /* Send data in TCP SYN */ #define MSG_CMSG_CLOEXEC 0x40000000 /* Set close_on_exec for file
diff --git a/include/net/sock.h b/include/net/sock.h
index 7c0632c7e870..e75e024b55d2 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h@@ -504,12 +504,14 @@ enum sk_pacing { int sk_set_peek_off(struct sock *sk, int val); -static inline int sk_peek_offset(struct sock *sk, int flags) +static inline int sk_peek_offset(struct sock *sk, int *flags) { - if (unlikely(flags & MSG_PEEK)) { + if (unlikely(*flags & MSG_PEEK)) { s32 off = READ_ONCE(sk->sk_peek_off); - if (off >= 0) + if (off >= 0) { + *flags |= MSG_PEEK_OFF; return off; + } } return 0;
diff --git a/net/core/datagram.c b/net/core/datagram.c
index ee5647bd91b3..91e1d014d64c 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c@@ -175,8 +175,8 @@ struct sk_buff *__skb_try_recv_from_queue(struct sock *sk, *last = queue->prev; skb_queue_walk(queue, skb) { if (flags & MSG_PEEK) { - if (_off >= skb->len && (skb->len || _off || - skb->peeked)) { + if (flags & MSG_PEEK_OFF && _off >= skb->len && + (skb->len || _off || skb->peeked)) { _off -= skb->len; continue; }
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index a7c804f73990..7e1bcd3500f4 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c@@ -1574,7 +1574,7 @@ 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 = off = sk_peek_offset(sk, &flags); skb = __skb_recv_udp(sk, flags, noblock, &peeked, &off, &err); if (!skb) return err;
--- Paolo