netdev - Re: [net-next PATCH] qdisc: 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]

Message-ID: <1409757727.26422.44.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Wed, 03 Sep 2014 08:22:07 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Jesper Dangaard Brouer <brouer@...hat.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Alexander Duyck <alexander.duyck@...il.com>,
	netdev@...r.kernel.org
Subject: Re: [net-next PATCH] qdisc: validate frames going through the
 direct_xmit path

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@...hat.com> wrote:
> 
> > On Wed, 03 Sep 2014 06:43:25 -0700
> > Eric Dumazet <eric.dumazet@...il.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


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