Re: [PATCH] PPPOE can kfree SKB twice (was Re: kernel panic problem. (smp, iptables?))

[Michal Ostrowski <mostrows@us.ibm.com>]

kuznet@ms2.inr.ac.ru writes:

> Hello!
> 
> SOme short comment on the patch:
> 
> 
> > -	dev_queue_xmit(skb);
> > +	/* The skb we are to transmit may be a copy (see above).  If
> > +	 * this fails, then the caller is responsible for the original
> > +	 * skb, otherwise we must free it.  Also if this fails we must
> > +	 * free the copy that we made.
> > +	 */
> > +
> > +	if (dev_queue_xmit(skb)<0) {
> 
> dev_queue_xmit _frees_ frame, not depending on return value.
> Return value is not a criterium to assume anything.
> 

My mistake.  It seemed perfectly reasonable at 6:00 am.  :-)

However, could we not have dev_queue_xmit behave as such (not free
frame on failure)?  That is, could we extend dev_queue_xmit to tell it
(optionally) that we want the skb back in case of failure?
dev_queue_xmit unconditionally frees the skb in any failure mode,
which is I would venture to say that we could do this.

The reason why I'm proposing this is that ppp_generic.c assumes that
the skb is still available after a transmission failure via pppoe.  To
support the semantics of dev_queue_xmit and ppp_generic we would have
to always copy skb's inside pppoe_xmit.  Then, if dev_queue_xmit fails
the original is deleted.

In the common case dev_queue_xmit will not fail, and so in that case
I'd like to have to avoid making a copy of the skb.

Michal Ostrowski
mostrows@speakeasy.net