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

[Jesper Dangaard Brouer <brouer@redhat.com>]

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