netdev - Re: [RFC][PATCH 1/3] net: per skb control messages

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: <200807241634.31614.opurdila@ixiacom.com>
Date:	Thu, 24 Jul 2008 16:34:31 +0300
From:	Octavian Purdila <opurdila@...acom.com>
To:	Herbert Xu <herbert@...dor.apana.org.au>
Cc:	netdev@...r.kernel.org
Subject: Re: [RFC][PATCH 1/3] net: per skb control messages

On Thursday 24 July 2008, Herbert Xu wrote:

Hi Herbert,

Thanks for taking the time for the review. 

>
> Putting it in the share info area makes its life-cycle management
> difficult.  This is also a major change to how we store skb control
> data.
>

Initially I tried putting the new member directly into the skb, but then I
run into issues with cloned packets, since I needed a reference
counter to know when to delete the cmsg queue.

I've noticed that the shared info already has a reference counter and I've 
though that taking advantage of it would be better then doing it myself. 

The shinfo seems to be the right place also because the cmsg queue can not be 
modified once the skb has been pushed into the network stack. It sort of is 
like data, only with special meaning.

Is this approach wrong? Or is there a better way of doing it?

> > @@ -567,6 +612,8 @@ static void copy_skb_header(struct sk_buff *new,
> > const struct sk_buff *old) skb_shinfo(new)->gso_size =
> > skb_shinfo(old)->gso_size;
> >        skb_shinfo(new)->gso_segs = skb_shinfo(old)->gso_segs;
> >        skb_shinfo(new)->gso_type = skb_shinfo(old)->gso_type;
> > +
> > +       skb_shinfo(new)->cmsg = skb_shinfo(old)->cmsg;
> > }
> >
> > /**
> > @@ -610,7 +657,8 @@ struct sk_buff *skb_copy(const struct sk_buff *skb,
> > gfp_t gfp_mask) BUG();
> >
> >        copy_skb_header(n, skb);
> > -       return n;
> > +
> > +       return skb_cmsg_copy(n, gfp_mask);
>
> What about the other callers of copy_skb_header, like pskb_copy?

>From what I understand, the shinfo remains shared, thus there is no need to do 
the copy here.

> In fact why isn't this in copy_skb_header?
>

In copy_skb_header we do a pointer copy. This should be enough for packets 
that are cloned or partially cloned.

The only problem is when we do a full copy of the packet, where shinfo should 
be also copied. skb_cmsg_copy is called only in these cases.

> Also what about pskb_expand_head?
>

I think this is only touching the skb header, not the shinfo. Thus we should 
be safe?

BTW: I have a dilemma with regard to fully copied skbs: should we copy the 
cmsg queue as well, or should we just prune it in the copy? I don't see a 
real usecase for doing the copy at this point, but since these is at core 
level, maybe it is a good idea to be conservative and do the copying?

Thanks,
tavi
--
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