[net,v2,0/3] net/sched: act_ipt bug fixes

Message

Florian Westphal June 8, 2023, 2:02 p.m. UTC

v2: add "Fixes" tags, no other changes, so retaining Simons RvB tag.

While checking if netfilter could be updated to replace selected
instances of NF_DROP with kfree_skb_reason+NF_STOLEN to improve
debugging info via drop monitor I found that act_ipt is incompatible
with such an approach.  Moreover, it lacks multiple sanity checks
to avoid certain code paths that make assumptions that the tc layer
doesn't meet, such as header sanity checks, availability of skb_dst
skb_nfct() and so on.

act_ipt test in the tc selftest still pass with this applied.

I think that we should consider removal of this module, while
this should take care of all problems, its ipv4 only and I don't
think there are any netfilter targets that lack a native tc
equivalent, even when ignoring bpf.

Florian Westphal (3):
  net/sched: act_ipt: add sanity checks on table name and hook locations
  net/sched: act_ipt: add sanity checks on skb before calling target
  net/sched: act_ipt: zero skb->cb before calling target

 net/sched/act_ipt.c | 70 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 63 insertions(+), 7 deletions(-)

Comments

Jamal Hadi Salim June 8, 2023, 4:50 p.m. UTC | #1

On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <[e-mail]> wrote:
>
> v2: add "Fixes" tags, no other changes, so retaining Simons RvB tag.
>
> While checking if netfilter could be updated to replace selected
> instances of NF_DROP with kfree_skb_reason+NF_STOLEN to improve
> debugging info via drop monitor I found that act_ipt is incompatible
> with such an approach.  Moreover, it lacks multiple sanity checks
> to avoid certain code paths that make assumptions that the tc layer
> doesn't meet, such as header sanity checks, availability of skb_dst
> skb_nfct() and so on.
>
> act_ipt test in the tc selftest still pass with this applied.
>
> I think that we should consider removal of this module, while
> this should take care of all problems, its ipv4 only and I don't
> think there are any netfilter targets that lack a native tc
> equivalent, even when ignoring bpf.
>

I am all for removing it - but i am worried there are users based on
past interactions. Will try to ping some users and see if they
actually were using it. I will send a patch to retire it if it looks
safe.
Unfortunately ipt suffered because we couldnt coordinate between the
netfilter and tc subsystem. In the old days so calling/callee bugs
fixed in the netfilter world find their way into ipt. At some point
that stopped. Also the user space interfacing suffered from similar
issues.

cheers,
jamal

> Florian Westphal (3):
>   net/sched: act_ipt: add sanity checks on table name and hook locations
>   net/sched: act_ipt: add sanity checks on skb before calling target
>   net/sched: act_ipt: zero skb->cb before calling target
>
>  net/sched/act_ipt.c | 70 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 63 insertions(+), 7 deletions(-)
>
> --
> 2.40.1
>

Florian Westphal June 8, 2023, 6:36 p.m. UTC | #2

Jamal Hadi Salim <[e-mail]> wrote:
> On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <[e-mail]> wrote:
> > I think that we should consider removal of this module, while
> > this should take care of all problems, its ipv4 only and I don't
> > think there are any netfilter targets that lack a native tc
> > equivalent, even when ignoring bpf.
> >
> 
> I am all for removing it - but i am worried there are users based on
> past interactions. Will try to ping some users and see if they
> actually were using it.

Thanks Jamal.  I'd also be interested in what xt module(s) are used,
if any.

> I will send a patch to retire it if it looks
> safe.

Thanks.