diff mbox series

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

Message ID 20230818181432.54283-1-jhs@mojatatu.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net,1/1] net/sched: fix a qdisc modification with ambigous command request | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1330 this patch: 1330
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 1351 this patch: 1351
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1353 this patch: 1353
netdev/checkpatch warning WARNING: 'ambigous' may be misspelled - perhaps 'ambiguous'? WARNING: Prefer 'fallthrough;' over fallthrough comment WARNING: Too many leading tabs - consider code refactoring WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline fail Was 0 now: 3

Commit Message

Jamal Hadi Salim Aug. 18, 2023, 6:14 p.m. UTC
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/
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_api.c | 55 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 13 deletions(-)

Comments

Vladimir Oltean Aug. 19, 2023, 2:30 p.m. UTC | #1
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
>
diff mbox series

Patch

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)
+{
+	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))
+							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),