Hi,
On Tue, 2018-07-17 at 10:24 -0700, Cong Wang wrote:
> If you goal is to get rid of skb_clone(), why not just do the following?
>
> if (tcf_mirred_is_act_redirect(m_eaction)) {
> skb2 = skb;
> } else {
> skb2 = skb_clone(skb, GFP_ATOMIC);
> if (!skb2)
> goto out;
> }
>
> For redirect, we return TC_ACT_SHOT, so upper layer should not
> touch the skb after that.
>
> What am I missing here?
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 <xiao...@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.
I think the only other option (beyond re-using ACT_MIRROR) is adding
another action value, and let the upper layer re-inject the packet
while handling such action (similar to what ACT_MIRROR currently does,
but preserving the current mirred semantic).
Cheers,
Paolo
