net: Validate frames going through the direct_xmit path

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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.