| Message ID | 20230915104156.3406380-1-make_ruc2021@163.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sched: drr: dont intepret cls results when asked to drop | expand |
On Fri, Sep 15, 2023 at 12:42 PM Ma Ke <make_ruc2021@163.com> wrote: > > If asked to drop a packet via TC_ACT_SHOT it is unsafe to > assume res.class contains a valid pointer. > > Signed-off-by: Ma Ke <make_ruc2021@163.com> > --- > net/sched/sch_drr.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c > index 19901e77cd3b..2b854cb6edf9 100644 > --- a/net/sched/sch_drr.c > +++ b/net/sched/sch_drr.c > @@ -309,6 +309,8 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch, > *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; > fl = rcu_dereference_bh(q->filter_list); > result = tcf_classify(skb, NULL, fl, &res, false); > + if (result == TC_ACT_SHOT) > + return NULL; > if (result >= 0) { > #ifdef CONFIG_NET_CLS_ACT > switch (result) { > -- > 2.37.2 > I do not see a bug, TC_ACT_SHOT is handled in the switch (result) just fine at line 320 ?
On 15/09/2023 09:55, Eric Dumazet wrote: > On Fri, Sep 15, 2023 at 12:42 PM Ma Ke <make_ruc2021@163.com> wrote: >> >> If asked to drop a packet via TC_ACT_SHOT it is unsafe to >> assume res.class contains a valid pointer. >> >> Signed-off-by: Ma Ke <make_ruc2021@163.com> >> --- >> net/sched/sch_drr.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c >> index 19901e77cd3b..2b854cb6edf9 100644 >> --- a/net/sched/sch_drr.c >> +++ b/net/sched/sch_drr.c >> @@ -309,6 +309,8 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch, >> *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; >> fl = rcu_dereference_bh(q->filter_list); >> result = tcf_classify(skb, NULL, fl, &res, false); >> + if (result == TC_ACT_SHOT) >> + return NULL; >> if (result >= 0) { >> #ifdef CONFIG_NET_CLS_ACT >> switch (result) { >> -- >> 2.37.2 >> > > I do not see a bug, TC_ACT_SHOT is handled in the switch (result) just fine > at line 320 ? Following the code path (with CONFIG_NET_CLS_ACT=n in mind), it looks like there are a couple of places which return TC_ACT_SHOT before calling any classifiers, which then would cause some qdiscs to look into a uninitialized 'struct tcf_result res'. I could be misreading it... But if it's the problem the author is trying to fix, the obvious way to do it would be: struct tcf_result res = {};
On Fri, Sep 15, 2023 at 5:03 PM Pedro Tammela <pctammela@mojatatu.com> wrote: > > On 15/09/2023 09:55, Eric Dumazet wrote: > > On Fri, Sep 15, 2023 at 12:42 PM Ma Ke <make_ruc2021@163.com> wrote: > >> > >> If asked to drop a packet via TC_ACT_SHOT it is unsafe to > >> assume res.class contains a valid pointer. > >> > >> Signed-off-by: Ma Ke <make_ruc2021@163.com> > >> --- > >> net/sched/sch_drr.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c > >> index 19901e77cd3b..2b854cb6edf9 100644 > >> --- a/net/sched/sch_drr.c > >> +++ b/net/sched/sch_drr.c > >> @@ -309,6 +309,8 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch, > >> *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; > >> fl = rcu_dereference_bh(q->filter_list); > >> result = tcf_classify(skb, NULL, fl, &res, false); > >> + if (result == TC_ACT_SHOT) > >> + return NULL; > >> if (result >= 0) { > >> #ifdef CONFIG_NET_CLS_ACT > >> switch (result) { > >> -- > >> 2.37.2 > >> > > > > I do not see a bug, TC_ACT_SHOT is handled in the switch (result) just fine > > at line 320 ? > > Following the code path (with CONFIG_NET_CLS_ACT=n in mind), it looks > like there are a couple of places which return TC_ACT_SHOT before > calling any classifiers, which then would cause some qdiscs to look into > a uninitialized 'struct tcf_result res'. > I could be misreading it... But if it's the problem the author is trying > to fix, the obvious way to do it would be: > struct tcf_result res = {}; CONFIG_NET_CLS_ACT=n, how come TC_ACT_SHOT could be used ? Can we get rid of CONFIG_NET_CLS_ACT, this seems obfuscation to me at this point.
On Fri, Sep 15, 2023 at 11:06 AM Eric Dumazet <edumazet@google.com> wrote: > > On Fri, Sep 15, 2023 at 5:03 PM Pedro Tammela <pctammela@mojatatu.com> wrote: > > > > On 15/09/2023 09:55, Eric Dumazet wrote: > > > On Fri, Sep 15, 2023 at 12:42 PM Ma Ke <make_ruc2021@163.com> wrote: > > >> > > >> If asked to drop a packet via TC_ACT_SHOT it is unsafe to > > >> assume res.class contains a valid pointer. > > >> > > >> Signed-off-by: Ma Ke <make_ruc2021@163.com> > > >> --- > > >> net/sched/sch_drr.c | 2 ++ > > >> 1 file changed, 2 insertions(+) > > >> > > >> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c > > >> index 19901e77cd3b..2b854cb6edf9 100644 > > >> --- a/net/sched/sch_drr.c > > >> +++ b/net/sched/sch_drr.c > > >> @@ -309,6 +309,8 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch, > > >> *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; > > >> fl = rcu_dereference_bh(q->filter_list); > > >> result = tcf_classify(skb, NULL, fl, &res, false); > > >> + if (result == TC_ACT_SHOT) > > >> + return NULL; > > >> if (result >= 0) { > > >> #ifdef CONFIG_NET_CLS_ACT > > >> switch (result) { > > >> -- > > >> 2.37.2 > > >> > > > > > > I do not see a bug, TC_ACT_SHOT is handled in the switch (result) just fine > > > at line 320 ? > > > > Following the code path (with CONFIG_NET_CLS_ACT=n in mind), it looks > > like there are a couple of places which return TC_ACT_SHOT before > > calling any classifiers, which then would cause some qdiscs to look into > > a uninitialized 'struct tcf_result res'. > > I could be misreading it... But if it's the problem the author is trying > > to fix, the obvious way to do it would be: > > struct tcf_result res = {}; > > CONFIG_NET_CLS_ACT=n, how come TC_ACT_SHOT could be used ? > > Can we get rid of CONFIG_NET_CLS_ACT, this seems obfuscation to me at > this point. The problem is the verdict vs return code are intermixed - not saying this was fixing anything useful. We discussed this in the past after/during commit caa4b35b4317d5147b3ab0fbdc9c075c7d2e9c12 Victor worked on a patch to resolve that. Victor, maybe revive that patch and post as RFC? cheers, jamal
On 15/09/2023 19:55, Jamal Hadi Salim wrote: > On Fri, Sep 15, 2023 at 11:06 AM Eric Dumazet <edumazet@google.com> wrote: >> >> On Fri, Sep 15, 2023 at 5:03 PM Pedro Tammela <pctammela@mojatatu.com> wrote: >>> >>> On 15/09/2023 09:55, Eric Dumazet wrote: >>>> On Fri, Sep 15, 2023 at 12:42 PM Ma Ke <make_ruc2021@163.com> wrote: >>>>> >>>>> If asked to drop a packet via TC_ACT_SHOT it is unsafe to >>>>> assume res.class contains a valid pointer. >>>>> >>>>> Signed-off-by: Ma Ke <make_ruc2021@163.com> >>>>> --- >>>>> net/sched/sch_drr.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c >>>>> index 19901e77cd3b..2b854cb6edf9 100644 >>>>> --- a/net/sched/sch_drr.c >>>>> +++ b/net/sched/sch_drr.c >>>>> @@ -309,6 +309,8 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch, >>>>> *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; >>>>> fl = rcu_dereference_bh(q->filter_list); >>>>> result = tcf_classify(skb, NULL, fl, &res, false); >>>>> + if (result == TC_ACT_SHOT) >>>>> + return NULL; >>>>> if (result >= 0) { >>>>> #ifdef CONFIG_NET_CLS_ACT >>>>> switch (result) { >>>>> -- >>>>> 2.37.2 >>>>> >>>> >>>> I do not see a bug, TC_ACT_SHOT is handled in the switch (result) just fine >>>> at line 320 ? >>> >>> Following the code path (with CONFIG_NET_CLS_ACT=n in mind), it looks >>> like there are a couple of places which return TC_ACT_SHOT before >>> calling any classifiers, which then would cause some qdiscs to look into >>> a uninitialized 'struct tcf_result res'. >>> I could be misreading it... But if it's the problem the author is trying >>> to fix, the obvious way to do it would be: >>> struct tcf_result res = {}; >> >> CONFIG_NET_CLS_ACT=n, how come TC_ACT_SHOT could be used ? >> >> Can we get rid of CONFIG_NET_CLS_ACT, this seems obfuscation to me at >> this point. > > The problem is the verdict vs return code are intermixed - not saying > this was fixing anything useful. > We discussed this in the past after/during commit > caa4b35b4317d5147b3ab0fbdc9c075c7d2e9c12 > Victor worked on a patch to resolve that. Victor, maybe revive that > patch and post as RFC? Okk, will review, rebase on top of net-next and post. cheers, Victor
diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c index 19901e77cd3b..2b854cb6edf9 100644 --- a/net/sched/sch_drr.c +++ b/net/sched/sch_drr.c @@ -309,6 +309,8 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch, *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; fl = rcu_dereference_bh(q->filter_list); result = tcf_classify(skb, NULL, fl, &res, false); + if (result == TC_ACT_SHOT) + return NULL; if (result >= 0) { #ifdef CONFIG_NET_CLS_ACT switch (result) {
If asked to drop a packet via TC_ACT_SHOT it is unsafe to assume res.class contains a valid pointer. Signed-off-by: Ma Ke <make_ruc2021@163.com> --- net/sched/sch_drr.c | 2 ++ 1 file changed, 2 insertions(+)