Re: [PATCH net v2 3/3] net/sched: act_ipt: zero skb->cb before calling target

[Florian Westphal <fw@strlen.de>]

Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <fw@strlen.de> wrote:
> >
> > xtables relies on skb being owned by ip stack, i.e. with ipv4
> > check in place skb->cb is supposed to be IPCB.
> >
> > I don't see an immediate problem (REJECT target cannot be used anymore
> > now that PRE/POSTROUTING hook validation has been fixed), but better be
> > safe than sorry.
> >
> > A much better patch would be to either mark act_ipt as
> > "depends on BROKEN" or remove it altogether. I plan to do this
> > for -next in the near future.
> 
> Let me handle this part please.

Sure, no problem.

> > This tc extension is broken in the sense that tc lacks an
> > equivalent of NF_STOLEN verdict.
> > With NF_STOLEN, target function takes complete ownership of skb, caller
> > cannot dereference it anymore.
> >
> > ACT_STOLEN cannot be used for this: it has a different meaning, caller
> > is allowed to dereference the skb.
> >
> 
> ACT_STOLEN requires that the target clones the packet and the caller
> to free the skb.

Makes sense, but if NF_STOLEN gets returned the skb is already released,
so we can't touch it anymore.

> > At this time NF_STOLEN won't be returned by any targets as far as I can
> > see, but this may change in the future.
> >
> > It might be possible to work around this via list of allowed
> > target extensions known to only return DROP or ACCEPT verdicts, but this
> > is error prone/fragile.
> 
> I didnt quiet follow why ACT_STOLEN if this action frees the packet
> and returns NF_STOLEN

We could emulate NF_STOLEN via ACT_STOLEN, yes, but we'd have to
skb_clone() unconditionally for every skb before calling the target
eval function...

Other alternatives I can think of:

- keep a list of "known safe" targets,
- annotate all accept-or-drop targets as "safe for act_ipt"
- make the skb shared before calling target function
- ensure that targets will never ever return NF_STOLEN

I dont really like any of these options :-)

At this time, targets return one of accept/drop/queue.

NF_QEUEUE will log an error and treats it like NF_ACCEPT,
so we are good at this time.