* Re: [net-next PATCH] qdisc: validate frames going through the direct_xmit path
2014-09-03 11:48 ` [net-next PATCH] qdisc: validate " Jesper Dangaard Brouer
@ 2014-09-03 13:43 ` Eric Dumazet
2014-09-03 13:52 ` Jesper Dangaard Brouer
2014-09-03 15:24 ` [net-next PATCH V2] " Jesper Dangaard Brouer
2014-09-03 15:56 ` [net-next PATCH V3] " Jesper Dangaard Brouer
2 siblings, 1 reply; 15+ messages in thread
From: Eric Dumazet @ 2014-09-03 13:43 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: David S. Miller, Alexander Duyck, netdev
On Wed, 2014-09-03 at 13:48 +0200, Jesper Dangaard Brouer wrote:
> In commit 50cbe9ab5f8d ("net: Validate xmit SKBs right when we pull them
> out of the qdisc") the validation code was moved out of dev_hard_start_xmit
> and into dequeue_skb. However this overlooked the fact that we do not
> always enqueue the skb onto a qdisc, if qdisc have flag TCQ_F_CAN_BYPASS.
>
> As a result Alex was seeing issues trying to connect to a vhost_net interface
> after this patch was applied.
>
> Added a call to validate_xmit_skb in __dev_xmit_skb(), in the code path
> for qdiscs with TCQ_F_CAN_BYPASS flag.
>
> Fixes: 50cbe9ab5f8d ("net: Validate xmit SKBs right when we pull them out of the qdisc")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
Jesper, you missed another spot, when there is no qdisc on the device.
__dev_queue_xmit() calls dev_hard_start_xmit() around line 2886
Could we try to not add a myriad of small patches ?
Some of us will need to backport all of them.
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [net-next PATCH] qdisc: validate frames going through the direct_xmit path
2014-09-03 13:43 ` Eric Dumazet
@ 2014-09-03 13:52 ` Jesper Dangaard Brouer
2014-09-03 14:26 ` Jesper Dangaard Brouer
0 siblings, 1 reply; 15+ messages in thread
From: Jesper Dangaard Brouer @ 2014-09-03 13:52 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Alexander Duyck, netdev, brouer
On Wed, 03 Sep 2014 06:43:25 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2014-09-03 at 13:48 +0200, Jesper Dangaard Brouer wrote:
> > In commit 50cbe9ab5f8d ("net: Validate xmit SKBs right when we pull them
> > out of the qdisc") the validation code was moved out of dev_hard_start_xmit
> > and into dequeue_skb. However this overlooked the fact that we do not
> > always enqueue the skb onto a qdisc, if qdisc have flag TCQ_F_CAN_BYPASS.
> >
> > As a result Alex was seeing issues trying to connect to a vhost_net interface
> > after this patch was applied.
> >
> > Added a call to validate_xmit_skb in __dev_xmit_skb(), in the code path
> > for qdiscs with TCQ_F_CAN_BYPASS flag.
> >
> > Fixes: 50cbe9ab5f8d ("net: Validate xmit SKBs right when we pull them out of the qdisc")
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > ---
>
> Jesper, you missed another spot, when there is no qdisc on the device.
>
> __dev_queue_xmit() calls dev_hard_start_xmit() around line 2886
>
> Could we try to not add a myriad of small patches ?
>
> Some of us will need to backport all of them.
I'm in the same backport situation ;-)
I'll send a V2 of this patch, with missed spot...
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [net-next PATCH] qdisc: validate frames going through the direct_xmit path
2014-09-03 13:52 ` Jesper Dangaard Brouer
@ 2014-09-03 14:26 ` Jesper Dangaard Brouer
2014-09-03 15:22 ` Eric Dumazet
0 siblings, 1 reply; 15+ messages in thread
From: Jesper Dangaard Brouer @ 2014-09-03 14:26 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Eric Dumazet, David S. Miller, Alexander Duyck, netdev
On Wed, 3 Sep 2014 15:52:13 +0200
Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> On Wed, 03 Sep 2014 06:43:25 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> >
> > Jesper, you missed another spot, when there is no qdisc on the device.
> >
> > __dev_queue_xmit() calls dev_hard_start_xmit() around line 2886
> >
> > Could we try to not add a myriad of small patches ?
> >
> > Some of us will need to backport all of them.
>
> I'm in the same backport situation ;-)
>
> I'll send a V2 of this patch, with missed spot...
(to avoid doing too many patches)
In the code below (from __dev_queue_xmit()), I've added validate_xmit_skb().
(dev_hard_start_xmit() can handle if it is a NULL ptr)
HARD_TX_LOCK(dev, txq, cpu);
if (!netif_xmit_stopped(txq)) {
skb = validate_xmit_skb(skb, dev);
__this_cpu_inc(xmit_recursion);
skb = dev_hard_start_xmit(skb, dev, txq, &rc);
__this_cpu_dec(xmit_recursion);
if (dev_xmit_complete(rc)) {
HARD_TX_UNLOCK(dev, txq);
goto out;
}
}
HARD_TX_UNLOCK(dev, txq);
net_crit_ratelimited("Virtual device %s asks to queue packet!\n",
dev->name);
BUT shouldn't we also handle if dev_hard_start_xmit() returns an skb pointer,
which means that there were skb's left on the skb list. Due to the rc
value "returned" in this case, we should see the net_crit_ratelimited()
msg, but we don't handle freeing these SKBs.
Any recommentations?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [net-next PATCH] qdisc: validate frames going through the direct_xmit path
2014-09-03 14:26 ` Jesper Dangaard Brouer
@ 2014-09-03 15:22 ` Eric Dumazet
0 siblings, 0 replies; 15+ messages in thread
From: Eric Dumazet @ 2014-09-03 15:22 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: David S. Miller, Alexander Duyck, netdev
On Wed, 2014-09-03 at 16:26 +0200, Jesper Dangaard Brouer wrote:
> On Wed, 3 Sep 2014 15:52:13 +0200
> Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
> > On Wed, 03 Sep 2014 06:43:25 -0700
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > >
> > > Jesper, you missed another spot, when there is no qdisc on the device.
> > >
> > > __dev_queue_xmit() calls dev_hard_start_xmit() around line 2886
> > >
> > > Could we try to not add a myriad of small patches ?
> > >
> > > Some of us will need to backport all of them.
> >
> > I'm in the same backport situation ;-)
> >
> > I'll send a V2 of this patch, with missed spot...
>
> (to avoid doing too many patches)
>
> In the code below (from __dev_queue_xmit()), I've added validate_xmit_skb().
> (dev_hard_start_xmit() can handle if it is a NULL ptr)
>
> HARD_TX_LOCK(dev, txq, cpu);
> if (!netif_xmit_stopped(txq)) {
> skb = validate_xmit_skb(skb, dev);
> __this_cpu_inc(xmit_recursion);
> skb = dev_hard_start_xmit(skb, dev, txq, &rc);
> __this_cpu_dec(xmit_recursion);
> if (dev_xmit_complete(rc)) {
> HARD_TX_UNLOCK(dev, txq);
> goto out;
> }
> }
> HARD_TX_UNLOCK(dev, txq);
> net_crit_ratelimited("Virtual device %s asks to queue packet!\n",
> dev->name);
> BUT shouldn't we also handle if dev_hard_start_xmit() returns an skb pointer,
> which means that there were skb's left on the skb list. Due to the rc
> value "returned" in this case, we should see the net_crit_ratelimited()
> msg, but we don't handle freeing these SKBs.
>
> Any recommentations?
>
First I would do the validate_xmit_skb() before HARD_TX_LOCK(dev, txq,
cpu)
Then, if we have skbs that could not be transmitted, we need to free
them (and possibly account the drops on the device tx_dropped). Logging
a one time message would be enough.
Thanks
^ permalink raw reply [flat|nested] 15+ messages in thread
* [net-next PATCH V2] qdisc: validate frames going through the direct_xmit path
2014-09-03 11:48 ` [net-next PATCH] qdisc: validate " Jesper Dangaard Brouer
2014-09-03 13:43 ` Eric Dumazet
@ 2014-09-03 15:24 ` Jesper Dangaard Brouer
2014-09-03 15:56 ` [net-next PATCH V3] " Jesper Dangaard Brouer
2 siblings, 0 replies; 15+ messages in thread
From: Jesper Dangaard Brouer @ 2014-09-03 15:24 UTC (permalink / raw)
To: Jesper Dangaard Brouer, David S. Miller, Alexander Duyck, netdev
Cc: Eric Dumazet
In commit 50cbe9ab5f8d ("net: Validate xmit SKBs right when we
pull them out of the qdisc") the validation code was moved out of
dev_hard_start_xmit and into dequeue_skb.
However this overlooked the fact that we do not always enqueue
the skb onto a qdisc. First situation is if qdisc have flag
TCQ_F_CAN_BYPASS and qdisc is empty. Second situation is if
there is no qdisc on the device, which is a common case for
software devices.
Originally spotted and inital patch by Alexander Duyck.
As a result Alex was seeing issues trying to connect to a
vhost_net interface after commit 50cbe9ab5f8d was applied.
Added a call to validate_xmit_skb() in __dev_xmit_skb(), in the
code path for qdiscs with TCQ_F_CAN_BYPASS flag, and in
__dev_queue_xmit() when no qdisc.
Fixes: 50cbe9ab5f8d ("net: Validate xmit SKBs right when we pull them out of the qdisc")
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
V2:
- Also handle no qdisc on the device situation
Posting a V2 quickly, if DaveM wants this fixed quickly.
net/core/dev.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 3774afc..8d5af9c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2739,7 +2739,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
qdisc_bstats_update(q, skb);
- if (sch_direct_xmit(skb, q, dev, txq, root_lock)) {
+ skb = validate_xmit_skb(skb, dev);
+ if (skb && sch_direct_xmit(skb, q, dev, txq, root_lock)) {
if (unlikely(contended)) {
spin_unlock(&q->busylock);
contended = false;
@@ -2882,6 +2883,7 @@ static int __dev_queue_xmit(struct sk_buff *skb, void *accel_priv)
HARD_TX_LOCK(dev, txq, cpu);
if (!netif_xmit_stopped(txq)) {
+ skb = validate_xmit_skb(skb, dev);
__this_cpu_inc(xmit_recursion);
skb = dev_hard_start_xmit(skb, dev, txq, &rc);
__this_cpu_dec(xmit_recursion);
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [net-next PATCH V3] qdisc: validate frames going through the direct_xmit path
2014-09-03 11:48 ` [net-next PATCH] qdisc: validate " Jesper Dangaard Brouer
2014-09-03 13:43 ` Eric Dumazet
2014-09-03 15:24 ` [net-next PATCH V2] " Jesper Dangaard Brouer
@ 2014-09-03 15:56 ` Jesper Dangaard Brouer
2014-09-03 16:08 ` Eric Dumazet
2014-09-04 3:43 ` David Miller
2 siblings, 2 replies; 15+ messages in thread
From: Jesper Dangaard Brouer @ 2014-09-03 15:56 UTC (permalink / raw)
To: Jesper Dangaard Brouer, David S. Miller, Alexander Duyck, netdev
Cc: Eric Dumazet
In commit 50cbe9ab5f8d ("net: Validate xmit SKBs right when we
pull them out of the qdisc") the validation code was moved out of
dev_hard_start_xmit and into dequeue_skb.
However this overlooked the fact that we do not always enqueue
the skb onto a qdisc. First situation is if qdisc have flag
TCQ_F_CAN_BYPASS and qdisc is empty. Second situation is if
there is no qdisc on the device, which is a common case for
software devices.
Originally spotted and inital patch by Alexander Duyck.
As a result Alex was seeing issues trying to connect to a
vhost_net interface after commit 50cbe9ab5f8d was applied.
Added a call to validate_xmit_skb() in __dev_xmit_skb(), in the
code path for qdiscs with TCQ_F_CAN_BYPASS flag, and in
__dev_queue_xmit() when no qdisc.
Also handle the error situation where dev_hard_start_xmit() could
return a skb list, and does not return dev_xmit_complete(rc) and
falls through to the kfree_skb(), in that situation it should
call kfree_skb_list().
Fixes: 50cbe9ab5f8d ("net: Validate xmit SKBs right when we pull them out of the qdisc")
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
V2:
- Also handle no qdisc on the device situation
V3:
- In __dev_queue_xmit() move validate_xmit_skb() up before
HARD_TX_LOCK and record tx_dropped if the validation failed.
- Use kfree_skb_list() instead of kfree_skb() in fallthrough err case.
net/core/dev.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 3774afc..2f3dbd6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2739,7 +2739,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
qdisc_bstats_update(q, skb);
- if (sch_direct_xmit(skb, q, dev, txq, root_lock)) {
+ skb = validate_xmit_skb(skb, dev);
+ if (skb && sch_direct_xmit(skb, q, dev, txq, root_lock)) {
if (unlikely(contended)) {
spin_unlock(&q->busylock);
contended = false;
@@ -2879,6 +2880,10 @@ static int __dev_queue_xmit(struct sk_buff *skb, void *accel_priv)
if (__this_cpu_read(xmit_recursion) > RECURSION_LIMIT)
goto recursion_alert;
+ skb = validate_xmit_skb(skb, dev);
+ if (!skb)
+ goto drop;
+
HARD_TX_LOCK(dev, txq, cpu);
if (!netif_xmit_stopped(txq)) {
@@ -2904,10 +2909,11 @@ recursion_alert:
}
rc = -ENETDOWN;
+drop:
rcu_read_unlock_bh();
atomic_long_inc(&dev->tx_dropped);
- kfree_skb(skb);
+ kfree_skb_list(skb);
return rc;
out:
rcu_read_unlock_bh();
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [net-next PATCH V3] qdisc: validate frames going through the direct_xmit path
2014-09-03 15:56 ` [net-next PATCH V3] " Jesper Dangaard Brouer
@ 2014-09-03 16:08 ` Eric Dumazet
2014-09-03 16:17 ` Jesper Dangaard Brouer
2014-09-04 3:43 ` David Miller
1 sibling, 1 reply; 15+ messages in thread
From: Eric Dumazet @ 2014-09-03 16:08 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: David S. Miller, Alexander Duyck, netdev
On Wed, 2014-09-03 at 17:56 +0200, Jesper Dangaard Brouer wrote:
> In commit 50cbe9ab5f8d ("net: Validate xmit SKBs right when we
> pull them out of the qdisc") the validation code was moved out of
> dev_hard_start_xmit and into dequeue_skb.
>
> However this overlooked the fact that we do not always enqueue
> the skb onto a qdisc. First situation is if qdisc have flag
> TCQ_F_CAN_BYPASS and qdisc is empty. Second situation is if
> there is no qdisc on the device, which is a common case for
> software devices.
>
> Originally spotted and inital patch by Alexander Duyck.
> As a result Alex was seeing issues trying to connect to a
> vhost_net interface after commit 50cbe9ab5f8d was applied.
>
> Added a call to validate_xmit_skb() in __dev_xmit_skb(), in the
> code path for qdiscs with TCQ_F_CAN_BYPASS flag, and in
> __dev_queue_xmit() when no qdisc.
>
> Also handle the error situation where dev_hard_start_xmit() could
> return a skb list, and does not return dev_xmit_complete(rc) and
> falls through to the kfree_skb(), in that situation it should
> call kfree_skb_list().
It seems that in this situation, we will return rc = -ENETDOWN,
I do not think this is the right error code. Not sure if that matters...
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [net-next PATCH V3] qdisc: validate frames going through the direct_xmit path
2014-09-03 16:08 ` Eric Dumazet
@ 2014-09-03 16:17 ` Jesper Dangaard Brouer
0 siblings, 0 replies; 15+ messages in thread
From: Jesper Dangaard Brouer @ 2014-09-03 16:17 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Alexander Duyck, netdev, brouer
On Wed, 03 Sep 2014 09:08:27 -0700 Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2014-09-03 at 17:56 +0200, Jesper Dangaard Brouer wrote:
[...]
> > Also handle the error situation where dev_hard_start_xmit() could
> > return a skb list, and does not return dev_xmit_complete(rc) and
> > falls through to the kfree_skb(), in that situation it should
> > call kfree_skb_list().
>
> It seems that in this situation, we will return rc = -ENETDOWN,
> I do not think this is the right error code. Not sure if that matters...
Yes, I noticed, but we have always returned this, in these kind of err
situations, so I didn't want to change that behavior.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [net-next PATCH V3] qdisc: validate frames going through the direct_xmit path
2014-09-03 15:56 ` [net-next PATCH V3] " Jesper Dangaard Brouer
2014-09-03 16:08 ` Eric Dumazet
@ 2014-09-04 3:43 ` David Miller
1 sibling, 0 replies; 15+ messages in thread
From: David Miller @ 2014-09-04 3:43 UTC (permalink / raw)
To: brouer; +Cc: alexander.duyck, netdev, eric.dumazet
From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Wed, 03 Sep 2014 17:56:09 +0200
> In commit 50cbe9ab5f8d ("net: Validate xmit SKBs right when we
> pull them out of the qdisc") the validation code was moved out of
> dev_hard_start_xmit and into dequeue_skb.
>
> However this overlooked the fact that we do not always enqueue
> the skb onto a qdisc. First situation is if qdisc have flag
> TCQ_F_CAN_BYPASS and qdisc is empty. Second situation is if
> there is no qdisc on the device, which is a common case for
> software devices.
>
> Originally spotted and inital patch by Alexander Duyck.
> As a result Alex was seeing issues trying to connect to a
> vhost_net interface after commit 50cbe9ab5f8d was applied.
>
> Added a call to validate_xmit_skb() in __dev_xmit_skb(), in the
> code path for qdiscs with TCQ_F_CAN_BYPASS flag, and in
> __dev_queue_xmit() when no qdisc.
>
> Also handle the error situation where dev_hard_start_xmit() could
> return a skb list, and does not return dev_xmit_complete(rc) and
> falls through to the kfree_skb(), in that situation it should
> call kfree_skb_list().
>
> Fixes: 50cbe9ab5f8d ("net: Validate xmit SKBs right when we pull them out of the qdisc")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Applied.
^ permalink raw reply [flat|nested] 15+ messages in thread