netdev - Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree

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: <20080807.034545.202633862.davem@davemloft.net>
Date:	Thu, 07 Aug 2008 03:45:45 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	jarkao2@...il.com
Cc:	jussi.kivilinna@...et.fi, kaber@...sh.net, netdev@...r.kernel.org
Subject: Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb

From: Jarek Poplawski <jarkao2@...il.com>
Date: Thu, 7 Aug 2008 10:31:30 +0000

> On Thu, Aug 07, 2008 at 03:10:58AM -0700, David Miller wrote:
> > From: Jarek Poplawski <jarkao2@...il.com>
> > Date: Thu, 7 Aug 2008 10:09:10 +0000
> ...
> > > After some checking it looks mostly OK to me, but one thing: in
> > > sch_gred gred_drop() calls qdisc_drop(), so now it needs kfree_skb().
> > > BTW, maybe it would be nicer to add __qdisc_drop() for these new
> > > things?
> > 
> > qdisc_drop() sets the new __NET_XMIT_KFREE bit, but sch_gred wants to
> > return NET_XMIT_CN, so I OR'd in the __NET_XMIT_KFREE bit there.
> 
> Hmm... I'm not sure we're thinking about the same function?

Indeed, you're right.

I've added the kfree_skb() call to gred_drop() in my local tree.

Thanks!

BTW, speaking of ->requeue(), I think we could easily eliminate
that thing.  It only exists because we allow drivers to kick things
back to us via ->hard_start_xmit() return values.

There are two cases:

1) NETDEV_TX_LOCKED... thanks to the bogon named LLTX

2) NETDEV_TX_BUSY, which is pretty much a bug

   %99.999 of the cases that return NETDEV_TX_BUSY are error
   conditions in the driver which log a message and we could
   just as validly drop the packet for this case

But anyways, LLTX isn't dying tomorrow as much as we'd like it go
away.

But we could cache the SKB instead of requeueing it, just like how
we handle the ->gso_skb right now.

In fact it seems we can just reuse the ->gso_skb Qdisc member for
this purpose.

Then all of the ->requeue() code and resulting complexity can just
go away.
--
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