netdev - Re: [PATCH] net: Validate frames going through the direct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 02 Sep 2014 19:46:34 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>,
	Alexander Duyck <alexander.h.duyck@...el.com>
CC:	netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH] net: Validate frames going through the direct_xmit path

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@...el.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html