netdev - Re: [PATCH] net: add destructor for skb data (rewritten)

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] [day] [month] [year] [list]

Message-Id: <200804200220.39985.rusty@rustcorp.com.au>
Date:	Sun, 20 Apr 2008 02:20:39 +1000
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org, maxk@...lcomm.com,
	herbert@...dor.apana.org.au
Subject: Re: [PATCH] net: add destructor for skb data (rewritten)

On Saturday 19 April 2008 19:35:24 David Miller wrote:
> From: Rusty Russell <rusty@...tcorp.com.au>
> Date: Fri, 18 Apr 2008 14:21:25 +1000
>
> > If we want to notify something when an skb is truly finished (such as
> > for tun vringfd support), we need a destructor on the data.
> >
> > This turns out to be slightly non-trivial as fragments from one skb
> > get copied to another skb: if the first skb has a destructor (or its
> > parent does) we need to keep a reference to it and destroy it only
> > when (all the) children are destroyed.  We add an 'orig' pointer to
> > the skb_shared_info to do this.
> >
> > But there's currently no way to get from the shinfo to the head (to
> > kfree it), so we add a 'len' field.  A better alternative to this
> > might be to move the skb_shared_info to before the head of the skb data.
> >
> > Note that the destructor is responsible for calling kfree: for the tun
> > device, this is critical since the destructor can be called from any
> > context and it has to do a copy_to_user, so it queues the skb.
> >
> > Signed-off-by: Rusty Russell <rusty@...tcorp.com.au>
>
> I'm mostly ambivalent but I will say I'm not happy about all of this
> extra state you're adding even though it's "only" to the SKB data
> shared-info struct and not sk_buff properly.

Me neither.  Moving the shared_info to the front of the data would reduce it 
to two fields for me (removing len and destructor arg), but I held off for 
now because this is a lesser change and possible for 2.6.26.

> Does this handle SKB frags of arbitrary depth?  SKB's can be nested to
> arbitrary depths via the frag mechanism.

It doesn't matter in this case.  It's the skb creator who sets the destructor, 
and wants it called when all pages in shinfo->frags[] are done with.  If it 
wanted to also include the frag_list in this lifetime, it would simply 
set ->orig on those skbs's shinfo to point back to the head shinfo, and 
adjust dataref accordingly.

As long as the anyone referencing a frags page from an skb into another sets 
the orig ptr & bumps dataref, this will work.  If someone kept a reference to 
a page and then freed the skb it game from, we're already broken.

You could think of it as a 'struct request' for networking.  Or not.

Cheers,
Rusty.
--
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