Re: kernel WARNING on skb_complete_tx_timestamp

["Keller, Jacob E" <jacob.e.keller@intel.com>]

Hi,

On Mon, 2015-04-13 at 16:31 +0000, Keller, Jacob E wrote:
> Hi Eric,

> > > This occurs because of the WARN_ON in kfree_skb which results from sock_hold(sk) and sock_put(sk)
> > > 
> > > I have a driver (fm10k) which receives a notification of Tx timestamp via an IRQ, and then when I call skb_complete_tx_timestamp I get this warning. I believe this is a result of calling sk_free, which the description *says* is ok to call in any context.. but then we get this warning.
> > > 
> > > I'm really not sure exactly how this situation occurred. Eventually we call kfree_skb() while we are in irq context which results in the warning.
> > 
> > At first look, there are some issues with this driver.
> > 
> 
> I should clarify here. I am working on fixing the issues with 1588 in
> this driver. The actual panic I got wasn't on the same code as currently
> in upstream. There are a lot of issues we're actively debugging now.
> However, your suggestion below does seem like it still applies.
> Hopefully the total set of fixes will be ready to post soon.
> 
> > fm10k_ts_tx_enqueue() is racy and seems also buggy, freeing wrong skb.
> > 
> > Could you try :
> > 
> > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_ptp.c
> > b/drivers/net/ethernet/intel/fm10k/fm10k_ptp.c
> > index
> > 02008e976d186f754470340089f344e781e9bb04..070d4f0b3c03bb0e31e216eb82d00f4fdcb4ea9f 100644
> > --- a/drivers/net/ethernet/intel/fm10k/fm10k_ptp.c
> > +++ b/drivers/net/ethernet/intel/fm10k/fm10k_ptp.c
> > @@ -70,16 +70,15 @@ void fm10k_ts_tx_enqueue(struct fm10k_intfc
> > *interface, struct sk_buff *skb)
> >  	 * if none are present then insert skb in tail of list
> >  	 */
> >  	skb = fm10k_ts_tx_skb(interface, FM10K_CB(clone)->fi.w.dglort);
> > -	if (!skb)
> > +	if (!skb) {
> > +		skb_shinfo(clone)->tx_flags |= SKBTX_IN_PROGRESS;
> >  		__skb_queue_tail(list, clone);
> > -
> > +	}
> >  	spin_unlock_irqrestore(&list->lock, flags);
> >  
> >  	/* if list is already has one then we just free the clone */
> >  	if (skb)
> > -		kfree_skb(skb);
> > -	else
> > -		skb_shinfo(clone)->tx_flags |= SKBTX_IN_PROGRESS;
> > +		kfree_skb(clone);
> 
> So free the clone instead of the original? I'm not entirely sure how
> this changes the flow, but I will give this a shot.
> 
> Regards,
> Jake
> 
> >  }
> >  
> >  void fm10k_ts_tx_hwtstamp(struct fm10k_intfc *interface, __le16 dglort,
> > 
> > 
> > 

Turns out this was already applied on my test out-of-tree driver I found
the stack dump on. This does need to get posted to the list, but it
isn't the cause of the problem.

Regards,
Jake