[Click] Packet::clone() potential problem

[Barry Gowan]

Hi Eddie, 
Thanks for the clarification on packet sharing
regarding uniqueify(). I had not spotted this beore. 

> Could you tell us how you actually modified the
> packet's data? 

In my own click element what I was doing was modifying
skb->data[12,13] to change the length field of my
cloned ethernet packets as well as changing skb->len. 

I'm not sure how I was getting away with this before,
but without modifying skb->data, I was simply
outputting packet->clone() to ToDevice to achieve a
'multiplication' of packets. As I understand it the
card driver code could have been free()ing my 
skb->data when the first clone was sent out. Sooner or
later this should have caused a kernel panic but I
never saw any problems with sending out
packet->clone()s until I tried modifying the data. 

It's all working ok now anyway, that's the main thing,

cheers,
BG

 --- Eddie Kohler <kohler at icir.org> wrote: > Hi Barry,
> 
> > May I suggest the addition of Packet::copy()
> similar
> > to Packet::clone() except that it uses skb_copy()
> > instead of skb_clone(). 
> > 
> > I was using Packet::clone() and it was causing me
> > problems since I was unaware that skb_clone()
> creates
> > an skb that shares the same skb->data as the
> original.
> > Thus, modifing the clone's skb->data before
> sending it
> > to ToDevice was also modifying the original
> skb->data.
> > The addition of Packet::copy() may make other
> people
> > aware of this potential problem. 
> 
> Could you tell us how you actually modified the
> packet's data? It shouldn't
> have been via Click Packet methods, since none of
> Packet's methods provide
> access to mutable data (the data() method returns a
> 'const unsigned char
> *', for example). To get mutable data, you need a
> WritablePacket.
> WritablePackets are guaranteed not to be shared.
> They are created by the
> 'WritablePacket *Packet::uniqueify()' method, which
> basically does an
> 'skb_copy' if necessary.
> 
> I'm guessing you used 'skb()' to get a copy of the
> actual sk_buff. That is
> not the "Click way". Keep yourself limited to
> Packet's methods if you can,
> refrain from casting away const, and you won't run
> into this problem.
> 
> Sharing of Packets is described in the user manual
> (click.info); HTML at
>
file:///home/kohler/www-lcdf/clickweb/doc/progman.html#Packet%20Sharing
> .
> 
> You can get the effect of "copy()" with
> "WritablePacket *q =
> p->clone()->uniqueify()". Nevertheless, if several
> people find that too
> verbose, we could provide a Packet::copy() that did
> exactly that.
> 
> Eddie 

________________________________________________________________________
Want to chat instantly with your online friends?  Get the FREE Yahoo!
Messenger http://uk.messenger.yahoo.com/