[PATCH net-next] loopback: clear pfmemalloc on outgoing skb's

[PATCH net-next] loopback: clear pfmemalloc on outgoing skb's

[Josef Bacik]

I was seeing random disconnects while testing NBD over loopback.  This turned
out to be because NBD sets pfmemalloc on it's socket, however the receiving side
is a user space application so does not have pfmemalloc set on its socket.  This
means that sk_filter_trim_cap will simply drop this packet, under the assumption
that the other side will simply retransmit.  Well we do retransmit, and then the
packet is just dropped again for the same reason.  To keep this from happening
simply clear skb->pfmemalloc on transmit so that we don't drop the packet on the
receive side.

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 drivers/net/loopback.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 1e05b7c..13c9126 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -81,6 +81,13 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
 	 */
 	skb_dst_force(skb);
 
+	/* If our transmitter was a pfmemalloc socket we need to clear
+	 * pfmemalloc here, otherwise the receiving socket may not be
+	 * pfmemalloc, and if this is a tcp packet then it'll get dropped and
+	 * all traffic will halt.
+	 */
+	skb->pfmemalloc = false;
+
 	skb->protocol = eth_type_trans(skb, dev);
 
 	/* it's OK to use per_cpu_ptr() because BHs are off */
-- 
2.7.4

Re: [PATCH net-next] loopback: clear pfmemalloc on outgoing skb's

[Eric Dumazet]

On Wed, 2017-02-01 at 16:04 -0500, Josef Bacik wrote:
> I was seeing random disconnects while testing NBD over loopback.  This turned
> out to be because NBD sets pfmemalloc on it's socket, however the receiving side
> is a user space application so does not have pfmemalloc set on its socket.  This
> means that sk_filter_trim_cap will simply drop this packet, under the assumption
> that the other side will simply retransmit.  Well we do retransmit, and then the
> packet is just dropped again for the same reason.  To keep this from happening
> simply clear skb->pfmemalloc on transmit so that we don't drop the packet on the
> receive side.
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  drivers/net/loopback.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> index 1e05b7c..13c9126 100644
> --- a/drivers/net/loopback.c
> +++ b/drivers/net/loopback.c
> @@ -81,6 +81,13 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
>  	 */
>  	skb_dst_force(skb);
>  
> +	/* If our transmitter was a pfmemalloc socket we need to clear
> +	 * pfmemalloc here, otherwise the receiving socket may not be
> +	 * pfmemalloc, and if this is a tcp packet then it'll get dropped and
> +	 * all traffic will halt.
> +	 */
> +	skb->pfmemalloc = false;
> +

I am not sure this is a proper fix.

Presumably if the socket was able to store packets in its write queue,
fact that it sends it to loopback or an Ethernet link should not matter.

Only in RX path the pfmemalloc thing is really important.

So I would rather not set skb->pfmemalloc for skbs allocated for the
write queue, and more exactly the fast clone.

This would actually speed up the stack a bit.

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 734c71468b013838516cfe8c744dcd0e797a6e2b..f91b81340dc5be80e0c26f9835d9192f35b75ad7 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -271,7 +271,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
                atomic_set(&fclones->fclone_ref, 1);
 
                fclones->skb2.fclone = SKB_FCLONE_CLONE;
-               fclones->skb2.pfmemalloc = pfmemalloc;
+               fclones->skb2.pfmemalloc = 0;
        }
 out:
        return skb;

Re: [PATCH net-next] loopback: clear pfmemalloc on outgoing skb's

[Eric Dumazet]

On Wed, 2017-02-01 at 15:38 -0800, Eric Dumazet wrote:
> On Wed, 2017-02-01 at 16:04 -0500, Josef Bacik wrote:
> > I was seeing random disconnects while testing NBD over loopback.  This turned
> > out to be because NBD sets pfmemalloc on it's socket, however the receiving side
> > is a user space application so does not have pfmemalloc set on its socket.  This
> > means that sk_filter_trim_cap will simply drop this packet, under the assumption
> > that the other side will simply retransmit.  Well we do retransmit, and then the
> > packet is just dropped again for the same reason.  To keep this from happening
> > simply clear skb->pfmemalloc on transmit so that we don't drop the packet on the
> > receive side.
> > 
> > Signed-off-by: Josef Bacik <jbacik@fb.com>
> > ---
> >  drivers/net/loopback.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> > index 1e05b7c..13c9126 100644
> > --- a/drivers/net/loopback.c
> > +++ b/drivers/net/loopback.c
> > @@ -81,6 +81,13 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
> >  	 */
> >  	skb_dst_force(skb);
> >  
> > +	/* If our transmitter was a pfmemalloc socket we need to clear
> > +	 * pfmemalloc here, otherwise the receiving socket may not be
> > +	 * pfmemalloc, and if this is a tcp packet then it'll get dropped and
> > +	 * all traffic will halt.
> > +	 */
> > +	skb->pfmemalloc = false;
> > +
> 
> I am not sure this is a proper fix.
> 
> Presumably if the socket was able to store packets in its write queue,
> fact that it sends it to loopback or an Ethernet link should not matter.
> 
> Only in RX path the pfmemalloc thing is really important.
> 
> So I would rather not set skb->pfmemalloc for skbs allocated for the
> write queue, and more exactly the fast clone.
> 
> This would actually speed up the stack a bit.
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 734c71468b013838516cfe8c744dcd0e797a6e2b..f91b81340dc5be80e0c26f9835d9192f35b75ad7 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -271,7 +271,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
>                 atomic_set(&fclones->fclone_ref, 1);
>  
>                 fclones->skb2.fclone = SKB_FCLONE_CLONE;
> -               fclones->skb2.pfmemalloc = pfmemalloc;
> +               fclones->skb2.pfmemalloc = 0;


It turns out this part was not needed in current kernel, because
__copy_skb_header() will copy pfmemalloc, since it is included in the
headers_start/headers_end section of skb.

So this patch is not solving your issue.

[PATCH net-next] net: remove useless pfmemalloc setting

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

When __alloc_skb() allocates an skb from fast clone cache,
setting pfmemalloc on the clone is not needed.

Clone will be properly initialized later at skb_clone() time,
including pfmemalloc field, as it is included in the
headers_start/headers_end section which is fully copied.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/skbuff.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 26c1344cc23e31d4248d23fc007bcab4b034c5c9..4f8f2a1a66b5a36705e8f9966f7abd751932dcc6 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -271,7 +271,6 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 		atomic_set(&fclones->fclone_ref, 1);
 
 		fclones->skb2.fclone = SKB_FCLONE_CLONE;
-		fclones->skb2.pfmemalloc = pfmemalloc;
 	}
 out:
 	return skb;

Re: [PATCH net-next] loopback: clear pfmemalloc on outgoing skb's

[Eric Dumazet]

On Wed, 2017-02-01 at 15:38 -0800, Eric Dumazet wrote:

> I am not sure this is a proper fix.
> 
> Presumably if the socket was able to store packets in its write queue,
> fact that it sends it to loopback or an Ethernet link should not matter.
> 
> Only in RX path the pfmemalloc thing is really important.
> 
> So I would rather not set skb->pfmemalloc for skbs allocated for the
> write queue, and more exactly the fast clone.

Also note that any looped packet would have the problem : veth, macvlan,
ipvlan, ...

Re: [PATCH net-next] loopback: clear pfmemalloc on outgoing skb's

[Josef Bacik]

On Wed, 2017-02-01 at 15:38 -0800, Eric Dumazet wrote:
> On Wed, 2017-02-01 at 16:04 -0500, Josef Bacik wrote:
> > 
> > I was seeing random disconnects while testing NBD over
> > loopback.  This turned
> > out to be because NBD sets pfmemalloc on it's socket, however the
> > receiving side
> > is a user space application so does not have pfmemalloc set on its
> > socket.  This
> > means that sk_filter_trim_cap will simply drop this packet, under
> > the assumption
> > that the other side will simply retransmit.  Well we do retransmit,
> > and then the
> > packet is just dropped again for the same reason.  To keep this
> > from happening
> > simply clear skb->pfmemalloc on transmit so that we don't drop the
> > packet on the
> > receive side.
> > 
> > Signed-off-by: Josef Bacik <jbacik@fb.com>
> > ---
> >  drivers/net/loopback.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> > index 1e05b7c..13c9126 100644
> > --- a/drivers/net/loopback.c
> > +++ b/drivers/net/loopback.c
> > @@ -81,6 +81,13 @@ static netdev_tx_t loopback_xmit(struct sk_buff
> > *skb,
> >  	 */
> >  	skb_dst_force(skb);
> >  
> > +	/* If our transmitter was a pfmemalloc socket we need to
> > clear
> > +	 * pfmemalloc here, otherwise the receiving socket may not
> > be
> > +	 * pfmemalloc, and if this is a tcp packet then it'll get
> > dropped and
> > +	 * all traffic will halt.
> > +	 */
> > +	skb->pfmemalloc = false;
> > +
> I am not sure this is a proper fix.
> 
> Presumably if the socket was able to store packets in its write
> queue,
> fact that it sends it to loopback or an Ethernet link should not
> matter.
> 
> Only in RX path the pfmemalloc thing is really important.
> 
> So I would rather not set skb->pfmemalloc for skbs allocated for the
> write queue, and more exactly the fast clone.
> 
> This would actually speed up the stack a bit.

The problem is we set skb->pfmemalloc a bunch of different places, such
as __skb_fill_page_desc, which appears to be used in both the RX and TX
path, so we can't just kill it there.  Do we want to go through and
audit each one, provide a way for callers to indicate if we care about
pfmemalloc and solve this problem that way?  I feel like that's more
likely to bite us in the ass down the line, and somebody who doesn't
know the context is going to come along and change it and regress us to
the current situation.  The only place this is a problem is with
loopback, and my change is contained to this one weird case.  Thanks,

Josef

Re: [PATCH net-next] loopback: clear pfmemalloc on outgoing skb's

[Eric Dumazet]

On Thu, 2017-02-02 at 10:56 -0500, Josef Bacik wrote:

> The problem is we set skb->pfmemalloc a bunch of different places, such
> as __skb_fill_page_desc, which appears to be used in both the RX and TX
> path, so we can't just kill it there.  Do we want to go through and
> audit each one, provide a way for callers to indicate if we care about
> pfmemalloc and solve this problem that way?  I feel like that's more
> likely to bite us in the ass down the line, and somebody who doesn't
> know the context is going to come along and change it and regress us to
> the current situation.  The only place this is a problem is with
> loopback, and my change is contained to this one weird case.  Thanks,

I mentioned this in another mail :

Same issue will happen with veth, or any kind of driver allowing skb
being given back to the stack in RX.

So your patch on loopback is not the definitive patch.

We probably should clear pf->memalloc directly in TCP write function.

Note that I clear it on the clone, not in original skb.

(It might be very useful to keep skb->pfmemalloc on original skbs in
write queue, at least for debugging purposes)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 8ce50dc3ab8cac821b8a2c3e0d31f0aa42f5c9d5..010280f1592d3bd195315882c364bdbbd4a1c2ec 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -944,6 +944,7 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
                        skb = skb_clone(skb, gfp_mask);
                if (unlikely(!skb))
                        return -ENOBUFS;
+               skb->pfmemalloc = 0;
        }
 
        inet = inet_sk(sk);

Re: [PATCH net-next] loopback: clear pfmemalloc on outgoing skb's

[Josef Bacik]

On Thu, 2017-02-02 at 09:06 -0800, Eric Dumazet wrote:
> On Thu, 2017-02-02 at 10:56 -0500, Josef Bacik wrote:
> 
> > 
> > The problem is we set skb->pfmemalloc a bunch of different places,
> > such
> > as __skb_fill_page_desc, which appears to be used in both the RX
> > and TX
> > path, so we can't just kill it there.  Do we want to go through and
> > audit each one, provide a way for callers to indicate if we care
> > about
> > pfmemalloc and solve this problem that way?  I feel like that's
> > more
> > likely to bite us in the ass down the line, and somebody who
> > doesn't
> > know the context is going to come along and change it and regress
> > us to
> > the current situation.  The only place this is a problem is with
> > loopback, and my change is contained to this one weird
> > case.  Thanks,
> I mentioned this in another mail :
> 
> Same issue will happen with veth, or any kind of driver allowing skb
> being given back to the stack in RX.
> 
> So your patch on loopback is not the definitive patch.
> 
> We probably should clear pf->memalloc directly in TCP write function.
> 
> Note that I clear it on the clone, not in original skb.
> 
> (It might be very useful to keep skb->pfmemalloc on original skbs in
> write queue, at least for debugging purposes)
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index
> 8ce50dc3ab8cac821b8a2c3e0d31f0aa42f5c9d5..010280f1592d3bd195315882c36
> 4bdbbd4a1c2ec 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -944,6 +944,7 @@ static int tcp_transmit_skb(struct sock *sk,
> struct sk_buff *skb, int clone_it,
>                         skb = skb_clone(skb, gfp_mask);
>                 if (unlikely(!skb))
>                         return -ENOBUFS;
> +               skb->pfmemalloc = 0;
>         }
>  
>         inet = inet_sk(sk);
> 
> 

Yup this fixes my problem, you can add

Acked-by: Josef Bacik <jbacik@fb.com>

when you send it.  Thanks,

Josef

Re: [PATCH net-next] net: remove useless pfmemalloc setting

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 01 Feb 2017 18:13:23 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> When __alloc_skb() allocates an skb from fast clone cache,
> setting pfmemalloc on the clone is not needed.
> 
> Clone will be properly initialized later at skb_clone() time,
> including pfmemalloc field, as it is included in the
> headers_start/headers_end section which is fully copied.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.

[PATCH net-next] tcp: clear pfmemalloc on outgoing skb

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Josef Bacik diagnosed following problem :

   I was seeing random disconnects while testing NBD over loopback.
   This turned out to be because NBD sets pfmemalloc on it's socket,
   however the receiving side is a user space application so does not
   have pfmemalloc set on its socket. This means that
   sk_filter_trim_cap will simply drop this packet, under the
   assumption that the other side will simply retransmit. Well we do
   retransmit, and then the packet is just dropped again for the same
   reason.

It seems the better way to address this problem is to clear pfmemalloc
in the TCP transmit path. pfmemalloc strict control really makes sense
on the receive path. 

Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Josef Bacik <jbacik@fb.com>
---
 net/ipv4/tcp_output.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 8ce50dc3ab8cac821b8a2c3e0d31f0aa42f5c9d5..a22545c8917cdc2382717e6176f7bb384e1d91f2 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -966,6 +966,13 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	 */
 	skb->ooo_okay = sk_wmem_alloc_get(sk) < SKB_TRUESIZE(1);
 
+	/* If we had to use memory reserve to allocate this skb,
+	 * this might cause drops if packet is looped back :
+	 * Other socket might not have SOCK_MEMALLOC.
+	 * Packets not looped back do not care about pfmemalloc.
+	 */
+	skb->pfmemalloc = 0;
+
 	skb_push(skb, tcp_header_size);
 	skb_reset_transport_header(skb);
 

Re: [PATCH net-next] tcp: clear pfmemalloc on outgoing skb

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 02 Feb 2017 20:40:08 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> Josef Bacik diagnosed following problem :
> 
>    I was seeing random disconnects while testing NBD over loopback.
>    This turned out to be because NBD sets pfmemalloc on it's socket,
>    however the receiving side is a user space application so does not
>    have pfmemalloc set on its socket. This means that
>    sk_filter_trim_cap will simply drop this packet, under the
>    assumption that the other side will simply retransmit. Well we do
>    retransmit, and then the packet is just dropped again for the same
>    reason.
> 
> It seems the better way to address this problem is to clear pfmemalloc
> in the TCP transmit path. pfmemalloc strict control really makes sense
> on the receive path. 
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Josef Bacik <jbacik@fb.com>

Applied.