* [PATCH net-next] net: fix __skb_try_recv_from_queue to return the old behavior
@ 2017-05-17 4:47 Andrei Vagin
2017-05-17 9:04 ` Paolo Abeni
0 siblings, 1 reply; 5+ messages in thread
From: Andrei Vagin @ 2017-05-17 4:47 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Andrei Vagin, Paolo Abeni, Eric Dumazet
This function has to return NULL on a error case, because there is a
separate error variable.
The offset has to be changed only if skb is returned
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: David S. Miller <davem@davemloft.net>
Fixes: 65101aeca522 ("net/sock: factor out dequeue/peek with offset cod")
Signed-off-by: Andrei Vagin <avagin@openvz.org>
---
net/core/datagram.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/net/core/datagram.c b/net/core/datagram.c
index a4592b4..bc46118 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -170,20 +170,21 @@ struct sk_buff *__skb_try_recv_from_queue(struct sock *sk,
struct sk_buff **last)
{
struct sk_buff *skb;
+ int _off = *off;
*last = queue->prev;
skb_queue_walk(queue, skb) {
if (flags & MSG_PEEK) {
- if (*off >= skb->len && (skb->len || *off ||
+ if (_off >= skb->len && (skb->len || _off ||
skb->peeked)) {
- *off -= skb->len;
+ _off -= skb->len;
continue;
}
if (!skb->len) {
skb = skb_set_peeked(skb);
if (unlikely(IS_ERR(skb))) {
*err = PTR_ERR(skb);
- return skb;
+ return NULL;
}
}
*peeked = 1;
@@ -193,6 +194,7 @@ struct sk_buff *__skb_try_recv_from_queue(struct sock *sk,
if (destructor)
destructor(sk, skb);
}
+ *off = _off;
return skb;
}
return NULL;
@@ -253,8 +255,6 @@ struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned int flags,
*peeked = 0;
do {
- int _off = *off;
-
/* Again only user level code calls this function, so nothing
* interrupt level will suddenly eat the receive_queue.
*
@@ -263,8 +263,10 @@ struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned int flags,
*/
spin_lock_irqsave(&queue->lock, cpu_flags);
skb = __skb_try_recv_from_queue(sk, queue, flags, destructor,
- peeked, &_off, err, last);
+ peeked, off, &error, last);
spin_unlock_irqrestore(&queue->lock, cpu_flags);
+ if (error)
+ goto no_packet;
if (skb)
return skb;
--
2.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net-next] net: fix __skb_try_recv_from_queue to return the old behavior 2017-05-17 4:47 [PATCH net-next] net: fix __skb_try_recv_from_queue to return the old behavior Andrei Vagin @ 2017-05-17 9:04 ` Paolo Abeni 2017-05-17 18:39 ` [PATCH net-next v2] " Andrei Vagin 0 siblings, 1 reply; 5+ messages in thread From: Paolo Abeni @ 2017-05-17 9:04 UTC (permalink / raw) To: Andrei Vagin, David S. Miller; +Cc: netdev, Eric Dumazet On Tue, 2017-05-16 at 21:47 -0700, Andrei Vagin wrote: > This function has to return NULL on a error case, because there is a > separate error variable. > > The offset has to be changed only if skb is returned > > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Eric Dumazet <edumazet@google.com> > Cc: David S. Miller <davem@davemloft.net> > Fixes: 65101aeca522 ("net/sock: factor out dequeue/peek with offset cod") > Signed-off-by: Andrei Vagin <avagin@openvz.org> > --- > net/core/datagram.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/net/core/datagram.c b/net/core/datagram.c > index a4592b4..bc46118 100644 > --- a/net/core/datagram.c > +++ b/net/core/datagram.c > @@ -170,20 +170,21 @@ struct sk_buff *__skb_try_recv_from_queue(struct sock *sk, > struct sk_buff **last) > { > struct sk_buff *skb; > + int _off = *off; > > *last = queue->prev; > skb_queue_walk(queue, skb) { > if (flags & MSG_PEEK) { > - if (*off >= skb->len && (skb->len || *off || > + if (_off >= skb->len && (skb->len || _off || > skb->peeked)) { > - *off -= skb->len; > + _off -= skb->len; > continue; > } > if (!skb->len) { > skb = skb_set_peeked(skb); > if (unlikely(IS_ERR(skb))) { > *err = PTR_ERR(skb); > - return skb; > + return NULL; > } > } > *peeked = 1; > @@ -193,6 +194,7 @@ struct sk_buff *__skb_try_recv_from_queue(struct sock *sk, > if (destructor) > destructor(sk, skb); > > } > + *off = _off; > return skb; > } > return NULL; > @@ -253,8 +255,6 @@ struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned int flags, > > *peeked = 0; > do { > - int _off = *off; > - > /* Again only user level code calls this function, so nothing > * interrupt level will suddenly eat the receive_queue. > * > @@ -263,8 +263,10 @@ struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned int flags, > */ > spin_lock_irqsave(&queue->lock, cpu_flags); > skb = __skb_try_recv_from_queue(sk, queue, flags, destructor, > - peeked, &_off, err, last); > + peeked, off, &error, last); > spin_unlock_irqrestore(&queue->lock, cpu_flags); > + if (error) > + goto no_packet; > if (skb) > return skb; > Thank you for catching this so early! If __skb_try_recv_from_queue() updates the offset if/only if the skb is returned, than we can also add something like the following ? (only compile tested, should not bring any functional changes, only code cleanup) BTW can you please share some entry pointer/walk-through for the CRIU tests ? I'd like to try to integrate them in my workflow, thank you! Paolo --- diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index 8ad5862..e65c7ed 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -1555,16 +1555,13 @@ struct sk_buff *__skb_recv_udp(struct sock *sk, unsigned int flags, error = -EAGAIN; *peeked = 0; do { - int _off = *off; - spin_lock_bh(&queue->lock); skb = __skb_try_recv_from_queue(sk, queue, flags, udp_skb_destructor, - peeked, &_off, err, + peeked, off, err, &last); if (skb) { spin_unlock_bh(&queue->lock); - *off = _off; return skb; } @@ -1578,20 +1575,17 @@ struct sk_buff *__skb_recv_udp(struct sock *sk, unsigned int flags, * the sk_receive_queue lock if fwd memory scheduling * is needed. */ - _off = *off; spin_lock(&sk_queue->lock); skb_queue_splice_tail_init(sk_queue, queue); skb = __skb_try_recv_from_queue(sk, queue, flags, udp_skb_dtor_locked, - peeked, &_off, err, + peeked, off, err, &last); spin_unlock(&sk_queue->lock); spin_unlock_bh(&queue->lock); - if (skb) { - *off = _off; + if (skb) return skb; - } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH net-next v2] net: fix __skb_try_recv_from_queue to return the old behavior 2017-05-17 9:04 ` Paolo Abeni @ 2017-05-17 18:39 ` Andrei Vagin 2017-05-18 9:53 ` Paolo Abeni 2017-05-18 14:33 ` David Miller 0 siblings, 2 replies; 5+ messages in thread From: Andrei Vagin @ 2017-05-17 18:39 UTC (permalink / raw) To: David S. Miller; +Cc: netdev, Andrei Vagin, Paolo Abeni, Eric Dumazet This function has to return NULL on a error case, because there is a separate error variable. The offset has to be changed only if skb is returned v2: fix udp code to not use an extra variable Cc: Paolo Abeni <pabeni@redhat.com> Cc: Eric Dumazet <edumazet@google.com> Cc: David S. Miller <davem@davemloft.net> Fixes: 65101aeca522 ("net/sock: factor out dequeue/peek with offset cod") Signed-off-by: Andrei Vagin <avagin@openvz.org> --- net/core/datagram.c | 14 ++++++++------ net/ipv4/udp.c | 12 +++--------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/net/core/datagram.c b/net/core/datagram.c index a4592b4..bc46118 100644 --- a/net/core/datagram.c +++ b/net/core/datagram.c @@ -170,20 +170,21 @@ struct sk_buff *__skb_try_recv_from_queue(struct sock *sk, struct sk_buff **last) { struct sk_buff *skb; + int _off = *off; *last = queue->prev; skb_queue_walk(queue, skb) { if (flags & MSG_PEEK) { - if (*off >= skb->len && (skb->len || *off || + if (_off >= skb->len && (skb->len || _off || skb->peeked)) { - *off -= skb->len; + _off -= skb->len; continue; } if (!skb->len) { skb = skb_set_peeked(skb); if (unlikely(IS_ERR(skb))) { *err = PTR_ERR(skb); - return skb; + return NULL; } } *peeked = 1; @@ -193,6 +194,7 @@ struct sk_buff *__skb_try_recv_from_queue(struct sock *sk, if (destructor) destructor(sk, skb); } + *off = _off; return skb; } return NULL; @@ -253,8 +255,6 @@ struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned int flags, *peeked = 0; do { - int _off = *off; - /* Again only user level code calls this function, so nothing * interrupt level will suddenly eat the receive_queue. * @@ -263,8 +263,10 @@ struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned int flags, */ spin_lock_irqsave(&queue->lock, cpu_flags); skb = __skb_try_recv_from_queue(sk, queue, flags, destructor, - peeked, &_off, err, last); + peeked, off, &error, last); spin_unlock_irqrestore(&queue->lock, cpu_flags); + if (error) + goto no_packet; if (skb) return skb; diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index 7bd56c9..278e707 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -1465,16 +1465,13 @@ struct sk_buff *__skb_recv_udp(struct sock *sk, unsigned int flags, error = -EAGAIN; *peeked = 0; do { - int _off = *off; - spin_lock_bh(&queue->lock); skb = __skb_try_recv_from_queue(sk, queue, flags, udp_skb_destructor, - peeked, &_off, err, + peeked, off, err, &last); if (skb) { spin_unlock_bh(&queue->lock); - *off = _off; return skb; } @@ -1488,20 +1485,17 @@ struct sk_buff *__skb_recv_udp(struct sock *sk, unsigned int flags, * the sk_receive_queue lock if fwd memory scheduling * is needed. */ - _off = *off; spin_lock(&sk_queue->lock); skb_queue_splice_tail_init(sk_queue, queue); skb = __skb_try_recv_from_queue(sk, queue, flags, udp_skb_dtor_locked, - peeked, &_off, err, + peeked, off, err, &last); spin_unlock(&sk_queue->lock); spin_unlock_bh(&queue->lock); - if (skb) { - *off = _off; + if (skb) return skb; - } busy_check: if (!sk_can_busy_loop(sk)) -- 2.9.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v2] net: fix __skb_try_recv_from_queue to return the old behavior 2017-05-17 18:39 ` [PATCH net-next v2] " Andrei Vagin @ 2017-05-18 9:53 ` Paolo Abeni 2017-05-18 14:33 ` David Miller 1 sibling, 0 replies; 5+ messages in thread From: Paolo Abeni @ 2017-05-18 9:53 UTC (permalink / raw) To: Andrei Vagin, David S. Miller; +Cc: netdev, Eric Dumazet On Wed, 2017-05-17 at 11:39 -0700, Andrei Vagin wrote: > This function has to return NULL on a error case, because there is a > separate error variable. > > The offset has to be changed only if skb is returned > > v2: fix udp code to not use an extra variable > > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Eric Dumazet <edumazet@google.com> > Cc: David S. Miller <davem@davemloft.net> > Fixes: 65101aeca522 ("net/sock: factor out dequeue/peek with offset cod") > Signed-off-by: Andrei Vagin <avagin@openvz.org> > --- > net/core/datagram.c | 14 ++++++++------ > net/ipv4/udp.c | 12 +++--------- > 2 files changed, 11 insertions(+), 15 deletions(-) > > diff --git a/net/core/datagram.c b/net/core/datagram.c > index a4592b4..bc46118 100644 > --- a/net/core/datagram.c > +++ b/net/core/datagram.c > @@ -170,20 +170,21 @@ struct sk_buff *__skb_try_recv_from_queue(struct sock *sk, > struct sk_buff **last) > { > struct sk_buff *skb; > + int _off = *off; > > *last = queue->prev; > skb_queue_walk(queue, skb) { > if (flags & MSG_PEEK) { > - if (*off >= skb->len && (skb->len || *off || > + if (_off >= skb->len && (skb->len || _off || > skb->peeked)) { > - *off -= skb->len; > + _off -= skb->len; > continue; > } > if (!skb->len) { > skb = skb_set_peeked(skb); > if (unlikely(IS_ERR(skb))) { > *err = PTR_ERR(skb); > - return skb; > + return NULL; > } > } > *peeked = 1; > @@ -193,6 +194,7 @@ struct sk_buff *__skb_try_recv_from_queue(struct sock *sk, > if (destructor) > destructor(sk, skb); > } > + *off = _off; > return skb; > } > return NULL; > @@ -253,8 +255,6 @@ struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned int flags, > > *peeked = 0; > do { > - int _off = *off; > - > /* Again only user level code calls this function, so nothing > * interrupt level will suddenly eat the receive_queue. > * > @@ -263,8 +263,10 @@ struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned int flags, > */ > spin_lock_irqsave(&queue->lock, cpu_flags); > skb = __skb_try_recv_from_queue(sk, queue, flags, destructor, > - peeked, &_off, err, last); > + peeked, off, &error, last); > spin_unlock_irqrestore(&queue->lock, cpu_flags); > + if (error) > + goto no_packet; > if (skb) > return skb; > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > index 7bd56c9..278e707 100644 > --- a/net/ipv4/udp.c > +++ b/net/ipv4/udp.c > @@ -1465,16 +1465,13 @@ struct sk_buff *__skb_recv_udp(struct sock *sk, unsigned int flags, > error = -EAGAIN; > *peeked = 0; > do { > - int _off = *off; > - > spin_lock_bh(&queue->lock); > skb = __skb_try_recv_from_queue(sk, queue, flags, > udp_skb_destructor, > - peeked, &_off, err, > + peeked, off, err, > &last); > if (skb) { > spin_unlock_bh(&queue->lock); > - *off = _off; > return skb; > } > > @@ -1488,20 +1485,17 @@ struct sk_buff *__skb_recv_udp(struct sock *sk, unsigned int flags, > * the sk_receive_queue lock if fwd memory scheduling > * is needed. > */ > - _off = *off; > spin_lock(&sk_queue->lock); > skb_queue_splice_tail_init(sk_queue, queue); > > skb = __skb_try_recv_from_queue(sk, queue, flags, > udp_skb_dtor_locked, > - peeked, &_off, err, > + peeked, off, err, > &last); > spin_unlock(&sk_queue->lock); > spin_unlock_bh(&queue->lock); > - if (skb) { > - *off = _off; > + if (skb) > return skb; > - } > > busy_check: > if (!sk_can_busy_loop(sk)) LGTM, thanks! Acked-by: Paolo Abeni <pabeni@redhat.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v2] net: fix __skb_try_recv_from_queue to return the old behavior 2017-05-17 18:39 ` [PATCH net-next v2] " Andrei Vagin 2017-05-18 9:53 ` Paolo Abeni @ 2017-05-18 14:33 ` David Miller 1 sibling, 0 replies; 5+ messages in thread From: David Miller @ 2017-05-18 14:33 UTC (permalink / raw) To: avagin; +Cc: netdev, pabeni, edumazet From: Andrei Vagin <avagin@openvz.org> Date: Wed, 17 May 2017 11:39:05 -0700 > This function has to return NULL on a error case, because there is a > separate error variable. > > The offset has to be changed only if skb is returned > > v2: fix udp code to not use an extra variable > > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Eric Dumazet <edumazet@google.com> > Cc: David S. Miller <davem@davemloft.net> > Fixes: 65101aeca522 ("net/sock: factor out dequeue/peek with offset cod") > Signed-off-by: Andrei Vagin <avagin@openvz.org> Applied. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-05-18 14:33 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-17 4:47 [PATCH net-next] net: fix __skb_try_recv_from_queue to return the old behavior Andrei Vagin 2017-05-17 9:04 ` Paolo Abeni 2017-05-17 18:39 ` [PATCH net-next v2] " Andrei Vagin 2017-05-18 9:53 ` Paolo Abeni 2017-05-18 14:33 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).