Re: [PATCH net 1/1] net/sched: fix a qdisc modification with ambigous command request

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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
>