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

[Simon Horman]

On Mon, Aug 21, 2023 at 03:13:05PM -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 parameterization 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@xxxxxxx> helped narrow it down.
> 
> v1->V2 changes:
> - remove "inline" function definition (Vladmir)
> - remove extrenous braces in branches (Vladmir)
> - change inline function names (Pedro)
> - Run tdc tests (Victor)
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: syzbot+a3618a167af2021433cd@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://lore.kernel.org/netdev/20230816225759.g25x76kmgzya2gei@skbuf/T/
> Tested-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> Tested-by: Victor Nogueira <victor@xxxxxxxxxxxx>
> Reviewed-by: Pedro Tammela <pctammela@xxxxxxxxxxxx>
> Reviewed-by: Victor Nogueira <victor@xxxxxxxxxxxx>
> Signed-off-by: Jamal Hadi Salim <jhs@xxxxxxxxxxxx>
> ---
>  net/sched/sch_api.c | 54 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 41 insertions(+), 13 deletions(-)
> 
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index aa6b1fe65151..4c51b8bef1b8 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 bool req_create_or_replace(struct nlmsghdr *n)
> +{
> +	return (n->nlmsg_flags & NLM_F_CREATE &&
> +		n->nlmsg_flags & NLM_F_REPLACE);
> +}
> +
> +static bool req_create_exclusive(struct nlmsghdr *n)
> +{
> +	return (n->nlmsg_flags & NLM_F_CREATE &&
> +		n->nlmsg_flags & NLM_F_EXCL);
> +}
> +
> +static bool req_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,36 @@ 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 (req_create_or_replace(n) ||
> +					    req_create_exclusive(n))
> +						goto create_n_graft;
> +					else
> +						if (req_change(n))

Hi Jamal,

a minor nit from my side, which also came up in v1:
this could be else if, avoiding one level of indentation.

> +							goto create_n_graft2;
> +				}
>  			}
>  		}
>  	} else {
> @@ -1698,6 +1725,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
> 
>