Messages in this thread Patch in this message | | | Date | Sat, 19 Aug 2023 17:30:15 +0300 | From | Vladimir Oltean <> | Subject | Re: [PATCH net 1/1] net/sched: fix a qdisc modification with ambigous command request |
| |
Hi Jamal,
There's a typo in the title: ambigous -> ambiguous
On Fri, Aug 18, 2023 at 02:14:32PM -0400, Jamal Hadi Salim wrote: > When replacing an existing root qdisc, with one that is of the same kind, the > request boils down to essentially a paremeterization change i.e not one that > requires allocation and grafting of a new qdisc. syzbot was able to create a > scenario which resulted in a taprio qdisc replacing an existing taprio qdisc > with a combination of NLM_F_CREATE, NLM_F_REPLACE and NLM_F_EXCL leading to > create and graft scenario. > The fix ensures that only when the qdisc kinds are different that we should > allow a create and graft, otherwise it goes into the "change" codepath. > > While at it, fix the code and comments to improve readability. > > While syzbot was able to create the issue, it did not zone on the root cause. > Analysis from Vladimir Oltean <vladimir.oltean@nxp.com> helped narrow it down. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Reported-by: syzbot+a3618a167af2021433cd@syzkaller.appspotmail.com > Closes:https://lore.kernel.org/netdev/20230816225759.g25x76kmgzya2gei@skbuf/T/
Space between Closes: tag and link.
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> --- > net/sched/sch_api.c | 55 ++++++++++++++++++++++++++++++++++----------- > 1 file changed, 42 insertions(+), 13 deletions(-) > > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c > index aa6b1fe65151..dd3db8608275 100644 > --- a/net/sched/sch_api.c > +++ b/net/sched/sch_api.c > @@ -1547,10 +1547,28 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n, > return 0; > } > > +static inline bool cmd_create_or_replace(struct nlmsghdr *n)
We tend to use "static inline" functions only when defining them in headers. In C files, we drop "inline" and let the compiler decide whether to inline or not.
> +{ > + return (n->nlmsg_flags & NLM_F_CREATE && > + n->nlmsg_flags & NLM_F_REPLACE); > +} > + > +static inline bool cmd_create_exclusive(struct nlmsghdr *n) > +{ > + return (n->nlmsg_flags & NLM_F_CREATE && > + n->nlmsg_flags & NLM_F_EXCL); > +} > + > +static inline bool cmd_change(struct nlmsghdr *n) > +{ > + return (!(n->nlmsg_flags & NLM_F_CREATE) && > + !(n->nlmsg_flags & NLM_F_REPLACE) && > + !(n->nlmsg_flags & NLM_F_EXCL)); > +} > + > /* > * Create/change qdisc. > */ > - > static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n, > struct netlink_ext_ack *extack) > { > @@ -1644,27 +1662,37 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n, > * > * We know, that some child q is already > * attached to this parent and have choice: > - * either to change it or to create/graft new one. > + * 1) change it or 2) create/graft new one. > + * If the requested qdisc kind is different > + * than the existing one, then we choose graft. > + * If they are the same then this is "change" > + * operation - just let it fallthrough.. > * > * 1. We are allowed to create/graft only > - * if CREATE and REPLACE flags are set. > + * if the request is explicitly stating > + * "please create if it doesn't exist". > * > - * 2. If EXCL is set, requestor wanted to say, > - * that qdisc tcm_handle is not expected > + * 2. If the request is to exclusive create > + * then the qdisc tcm_handle is not expected > * to exist, so that we choose create/graft too. > * > * 3. The last case is when no flags are set. > + * This will happen when for example tc > + * utility issues a "change" command. > * Alas, it is sort of hole in API, we > * cannot decide what to do unambiguously. > - * For now we select create/graft, if > - * user gave KIND, which does not match existing. > + * For now we select create/graft. > */ > - if ((n->nlmsg_flags & NLM_F_CREATE) && > - (n->nlmsg_flags & NLM_F_REPLACE) && > - ((n->nlmsg_flags & NLM_F_EXCL) || > - (tca[TCA_KIND] && > - nla_strcmp(tca[TCA_KIND], q->ops->id)))) > - goto create_n_graft; > + if (tca[TCA_KIND] && > + nla_strcmp(tca[TCA_KIND], q->ops->id)) { > + if (cmd_create_or_replace(n) || > + cmd_create_exclusive(n)) { > + goto create_n_graft; > + } else { > + if (cmd_change(n))
else if cmd_change()
thus the code block under the qdisc kind comparison can become:
if (cmd_create_or_replace(n) || cmd_create_exclusive(n)) goto create_n_graft; else if (cmd_change(n)) goto create_n_graft2;
(no extra brackets are needed)
> + goto create_n_graft2; > + } > + } > } > } > } else { > @@ -1698,6 +1726,7 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n, > NL_SET_ERR_MSG(extack, "Qdisc not found. To create specify NLM_F_CREATE flag"); > return -ENOENT; > } > +create_n_graft2: > if (clid == TC_H_INGRESS) { > if (dev_ingress_queue(dev)) { > q = qdisc_create(dev, dev_ingress_queue(dev), > -- > 2.34.1 >
| |