Re: [RFC net-next 1/2] net: sched: refactor reinsert action

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Mon, 17 Jun 2019 19:43:53 +0100, Edward Cree wrote:
> On 14/06/2019 15:33, John Hurley wrote:
> > Instead of
> > returning TC_ACT_REINSERT, change the type to the new TC_ACT_CONSUMED
> > which tells the caller that the packet has been stolen by another process
> > and that no consume call is required.  
> Possibly a dumb question, but why does this need a new CONSUMED rather
>  than, say, taking an additional ref and returning TC_ACT_STOLEN?

Is it okay to reinsert a shared skb into the stack?  In particular this
looks a little scary:

int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
		     gfp_t gfp_mask)
{
	int i, osize = skb_end_offset(skb);
	int size = osize + nhead + ntail;
	long off;
	u8 *data;

	BUG_ON(nhead < 0);

	BUG_ON(skb_shared(skb));
	^^^^^^^^^^^^^^^^^^^^^^^^

Actually looking for Paolo's address to add him to CC I found that he
said at the time:

  With ACT_SHOT caller/upper layer will free the skb, too. We will have
  an use after free (from either the upper layer and the xmit device).
  Similar issues with STOLEN, TRAP, etc.

  In the past, Changli Gao attempted to avoid the clone incrementing the
  skb usage count:

  commit 210d6de78c5d7c785fc532556cea340e517955e1
  Author: Changli Gao <xiaosuo@gmail.com>
  Date:   Thu Jun 24 16:25:12 2010 +0000

      act_mirred: don't clone skb when skb isn't shared

  but some/many device drivers expect an skb usage count of 1, and that
  caused ooops and was revered.

:)