[PATCH] net: Validate frames going through the direct_xmit path

[PATCH] net: Validate frames going through the direct_xmit path

[Alexander Duyck]

In commit 50cbe9ab5f8d92d2d4a327b56e96559d8f63a1fa "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.

As a result I was seeing issues trying to connect to a vhost_net interface
after this patch was applied.  To resolve the issue I have added a call to
validate_xmit_skb in sched_direct_xmit and this seems to have resolved the
issue by restoring the validation to this xmit path.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 net/sched/sch_generic.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index a8bf9f9..203ee65 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -128,8 +128,13 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
 	spin_unlock(root_lock);
 
 	HARD_TX_LOCK(dev, txq, smp_processor_id());
-	if (!netif_xmit_frozen_or_stopped(txq))
-		skb = dev_hard_start_xmit(skb, dev, txq, &ret);
+	if (!netif_xmit_frozen_or_stopped(txq)) {
+		skb = validate_xmit_skb(skb, dev);
+		if (!skb)
+			ret = NETDEV_TX_OK;
+		else
+			skb = dev_hard_start_xmit(skb, dev, txq, &ret);
+	}
 
 	HARD_TX_UNLOCK(dev, txq);
 

Re: [PATCH] net: Validate frames going through the direct_xmit path

[Eric Dumazet]

On Tue, 2014-09-02 at 18:55 -0400, Alexander Duyck wrote:
> In commit 50cbe9ab5f8d92d2d4a327b56e96559d8f63a1fa "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.
> 
> As a result I was seeing issues trying to connect to a vhost_net interface
> after this patch was applied.  To resolve the issue I have added a call to
> validate_xmit_skb in sched_direct_xmit and this seems to have resolved the
> issue by restoring the validation to this xmit path.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>  net/sched/sch_generic.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index a8bf9f9..203ee65 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -128,8 +128,13 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
>  	spin_unlock(root_lock);
>  
>  	HARD_TX_LOCK(dev, txq, smp_processor_id());
> -	if (!netif_xmit_frozen_or_stopped(txq))
> -		skb = dev_hard_start_xmit(skb, dev, txq, &ret);
> +	if (!netif_xmit_frozen_or_stopped(txq)) {
> +		skb = validate_xmit_skb(skb, dev);
> +		if (!skb)
> +			ret = NETDEV_TX_OK;
> +		else
> +			skb = dev_hard_start_xmit(skb, dev, txq, &ret);
> +	}
>  
>  	HARD_TX_UNLOCK(dev, txq);
>  

This looks very weird.

Calling validate_xmit_skb() twice per packet is not needed in the case
sch_direct_xmit() is called from qdisc_restart()

This will add bad branch prediction at very minimum.

This is a TCQ_F_CAN_BYPASS issue that should be fixed there.

Re: [PATCH] net: Validate frames going through the direct_xmit path

[Alexander Duyck]

On 09/02/2014 04:30 PM, Eric Dumazet wrote:
> On Tue, 2014-09-02 at 18:55 -0400, Alexander Duyck wrote:
>> In commit 50cbe9ab5f8d92d2d4a327b56e96559d8f63a1fa "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.
>>
>> As a result I was seeing issues trying to connect to a vhost_net interface
>> after this patch was applied.  To resolve the issue I have added a call to
>> validate_xmit_skb in sched_direct_xmit and this seems to have resolved the
>> issue by restoring the validation to this xmit path.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> ---
>>  net/sched/sch_generic.c |    9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>> index a8bf9f9..203ee65 100644
>> --- a/net/sched/sch_generic.c
>> +++ b/net/sched/sch_generic.c
>> @@ -128,8 +128,13 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
>>  	spin_unlock(root_lock);
>>  
>>  	HARD_TX_LOCK(dev, txq, smp_processor_id());
>> -	if (!netif_xmit_frozen_or_stopped(txq))
>> -		skb = dev_hard_start_xmit(skb, dev, txq, &ret);
>> +	if (!netif_xmit_frozen_or_stopped(txq)) {
>> +		skb = validate_xmit_skb(skb, dev);
>> +		if (!skb)
>> +			ret = NETDEV_TX_OK;
>> +		else
>> +			skb = dev_hard_start_xmit(skb, dev, txq, &ret);
>> +	}
>>  
>>  	HARD_TX_UNLOCK(dev, txq);
>>  
> 
> This looks very weird.

It's ugly, I will admit it.  It was a quick hack to fix the issue I had
been seeing as it was in my way.

> Calling validate_xmit_skb() twice per packet is not needed in the case
> sch_direct_xmit() is called from qdisc_restart()

My bad, I overlooked that sch_direct_xmit is called by qdisc_restart.

> This will add bad branch prediction at very minimum.
> 
> This is a TCQ_F_CAN_BYPASS issue that should be fixed there.

Actually it looks like there are several issues.  One is the bypass
problem which is the major issue. Another side effect of the original
patch is that a bad frame will cause us to exit __qdisc_run prematurely
even if other frames are still in the qdisc.

Alternative patches always welcome. :-)  My goal at this point is to
just have my vhost_net interface work so I can get back to my other
development work.  I will submit a v2 in the morning if I don't see
anything.

Alex

Re: [PATCH] net: Validate frames going through the direct_xmit path

[Eric Dumazet]

On Tue, 2014-09-02 at 19:46 -0700, Alexander Duyck wrote:

> Actually it looks like there are several issues.  One is the bypass
> problem which is the major issue. Another side effect of the original
> patch is that a bad frame will cause us to exit __qdisc_run prematurely
> even if other frames are still in the qdisc.

Hmm... maybe a the following would fix that ?

Also note we lack counters tracking these kind of events (dropped count)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 5b261e91bdbd..e051fdf95783 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -71,9 +71,13 @@ static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
 			skb = NULL;
 	} else {
 		if (!(q->flags & TCQ_F_ONETXQUEUE) || !netif_xmit_frozen_or_stopped(txq)) {
+dequeue:
 			skb = q->dequeue(q);
-			if (skb)
+			if (skb) {
 				skb = validate_xmit_skb(skb, qdisc_dev(q));
+				if (!skb)
+					goto dequeue;
+			}
 		}
 	}
 

Re: [PATCH] net: Validate frames going through the direct_xmit path

[Jesper Dangaard Brouer]

On Tue, 02 Sep 2014 19:46:34 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:

> Alternative patches always welcome. :-)  My goal at this point is to
> just have my vhost_net interface work so I can get back to my other
> development work.  I will submit a v2 in the morning if I don't see
> anything.

I've posted a followup, and gave you the SoB.

-- 
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

[net-next PATCH] qdisc: validate frames going through the direct_xmit path

[Jesper Dangaard Brouer]

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>
---

 net/core/dev.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 3774afc..5bbac90 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;

Re: [net-next PATCH] qdisc: validate frames going through the direct_xmit path

[Eric Dumazet]

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.

Re: [net-next PATCH] qdisc: validate frames going through the direct_xmit path

[Jesper Dangaard 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

Re: [net-next PATCH] qdisc: validate frames going through the direct_xmit path

[Jesper Dangaard Brouer]

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

Re: [net-next PATCH] qdisc: validate frames going through the direct_xmit path

[Eric Dumazet]

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

[net-next PATCH V2] qdisc: validate frames going through the direct_xmit path

[Jesper Dangaard Brouer]

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);

[net-next PATCH V3] qdisc: validate frames going through the direct_xmit path

[Jesper Dangaard Brouer]

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();

Re: [net-next PATCH V3] qdisc: validate frames going through the direct_xmit path

[Eric Dumazet]

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...

Re: [net-next PATCH V3] qdisc: validate frames going through the direct_xmit path

[Jesper Dangaard 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

Re: [net-next PATCH V3] qdisc: validate frames going through the direct_xmit path

[David Miller]

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.