Message ID | 20230531141556.1637341-1-lee@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp2936089vqr; Wed, 31 May 2023 07:48:08 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4CqLYhQzkjhyHKNNmt4ChR1Rq33fJwBk8V6dm+5U0v6hDBWWqJZxnt5DPupK0pndWy5Obb X-Received: by 2002:a05:6358:6f8e:b0:123:5d15:a5e1 with SMTP id s14-20020a0563586f8e00b001235d15a5e1mr2269231rwn.19.1685544488459; Wed, 31 May 2023 07:48:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685544488; cv=none; d=google.com; s=arc-20160816; b=a0yRj2cXhoNcYeT/G8D98s7d/t6X8dtK7yz1AX1/fs+B17WLCxw7yfr4/7zj2QENZ/ fJZ6xyql4e4ZSv61fTrqqgbXA+mYmMweT9cMnjZwjRjytE/2l1tuORnQk9w/vkiOs6pB wOj81Vv0BkB1CTCooBxC7E2LUjUFTfxRfkqOszRkGpVxrfRRz4h+EU0cq4PmbeGOI3eB Syc0sR4e0zYpqOnox/wD+anRz2zi3BGfB1nip6oXkNSB6DX/y6rhfmcl5y8XUTdsj6HG 7TcOAidPGfmLSJSrTYOJn940u1kb36/nzl0LC+Q0njVsaONZPDrqRnB/UfR9yRHnjZHu mFWQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=fM1adVsvY3OXCdwF47etYQpS4ySQNa/9fZEXlHqJOQs=; b=liMLJuznLXx2lOEMXi98n/JrgtudLWPB8dQSUOjHqbo3sfddhnk1Dn/E9Q0+uLm6eG BapGrB6odQHKogqx/qv6GTcggmxQfv3Frxhq6njszQjzmwSZotI8lGa1SA/0+bfTay42 3GW9ik53c5yb0k7gI/gGHEICCD1DVwrnkBH7no+SwCOpoqDZ/Sf9CdzLjLD66+Q2Nq7k X8fLrRB+IHPecENmeDiQo+Ca3962kz2MuNlhbtxyWf8zjSrJdzRXm6+4hk8HXH0Iea8q LYwkIRtfVbW9qRp4z1/kf4IHqpeH0K64rsBxYWDqZjuuYnhB+XfzgKATxTOrxafC/cuo shPg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=mO68rUpe; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id f18-20020a17090ace1200b00257a8dc0348si1083075pju.75.2023.05.31.07.47.54; Wed, 31 May 2023 07:48:08 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=mO68rUpe; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237434AbjEaOSv (ORCPT <rfc822;andrewvogler123@gmail.com> + 99 others); Wed, 31 May 2023 10:18:51 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33090 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237504AbjEaOSg (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 31 May 2023 10:18:36 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9FCA7137 for <linux-kernel@vger.kernel.org>; Wed, 31 May 2023 07:16:58 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 18020634C6 for <linux-kernel@vger.kernel.org>; Wed, 31 May 2023 14:16:26 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2F541C433D2; Wed, 31 May 2023 14:16:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685542585; bh=6xPVFqexSIW7Z9IDBUUq4nFv5EZETSZ0A3H+jexY8Ug=; h=From:To:Cc:Subject:Date:From; b=mO68rUpe21FDD87sqVUTxkHqsjWPBsyqdMq9X0kKabvZb5rCQuye9Ls2ovHj46Esm toJZVU3bPGB+7TO9sKR1unvcK2cqZJgibVtbPqNtD2CxdQtJ7nCuj713Qj0a3hCj5m C4cHGoMroa63d+q6HIrRBBG+XGn2acHkI52BrjqwqN2jU0upMi6Rg4sHrprNQnrgFL +xSP8hbglakFAPuRQ1UmjU7lZLiJWAXqkPSyzGSJ62wQrnArUxbaUvG8LBu9Jsqb4z syWYHiynewVeLFY5S9kOMIrPdz4dNfwP0uMp1x2RV12GwAJntAmp5Iqsrcydl02Y8a 2cj1fk/xnT+Qw== From: Lee Jones <lee@kernel.org> To: lee@kernel.org, jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org, stable@kernel.org Subject: [PATCH 1/1] net/sched: cls_u32: Fix reference counter leak leading to overflow Date: Wed, 31 May 2023 15:15:56 +0100 Message-ID: <20230531141556.1637341-1-lee@kernel.org> X-Mailer: git-send-email 2.41.0.rc0.172.g3f132b7071-goog MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1767421497052937241?= X-GMAIL-MSGID: =?utf-8?q?1767421497052937241?= |
Series |
[1/1] net/sched: cls_u32: Fix reference counter leak leading to overflow
|
|
Commit Message
Lee Jones
May 31, 2023, 2:15 p.m. UTC
In the event of a failure in tcf_change_indev(), u32_set_parms() will
immediately return without decrementing the recently incremented
reference counter. If this happens enough times, the counter will
rollover and the reference freed, leading to a double free which can be
used to do 'bad things'.
Cc: stable@kernel.org # v4.14+
Signed-off-by: Lee Jones <lee@kernel.org>
---
net/sched/cls_u32.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Comments
On Wed, May 31, 2023 at 4:16 PM Lee Jones <lee@kernel.org> wrote: > > In the event of a failure in tcf_change_indev(), u32_set_parms() will > immediately return without decrementing the recently incremented > reference counter. If this happens enough times, the counter will > rollover and the reference freed, leading to a double free which can be > used to do 'bad things'. > > Cc: stable@kernel.org # v4.14+ Please add a Fixes: tag. > Signed-off-by: Lee Jones <lee@kernel.org> > --- > net/sched/cls_u32.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c > index 4e2e269f121f8..fad61ca5e90bf 100644 > --- a/net/sched/cls_u32.c > +++ b/net/sched/cls_u32.c > @@ -762,8 +762,11 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, > if (tb[TCA_U32_INDEV]) { > int ret; > ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); This call should probably be done earlier in the function, next to tcf_exts_validate_ex() Otherwise we might ask why the tcf_bind_filter() does not need to be undone. Something like: diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c index 4e2e269f121f8a301368b9783753e055f5af6a4e..ac957ff2216ae18bcabdd3af3b0e127447ef8f91 100644 --- a/net/sched/cls_u32.c +++ b/net/sched/cls_u32.c @@ -718,13 +718,18 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, struct nlattr *est, u32 flags, u32 fl_flags, struct netlink_ext_ack *extack) { - int err; + int err, ifindex = -1; err = tcf_exts_validate_ex(net, tp, tb, est, &n->exts, flags, fl_flags, extack); if (err < 0) return err; + if (tb[TCA_U32_INDEV]) { + ifindex = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); + if (ifindex < 0) + return -EINVAL; + } if (tb[TCA_U32_LINK]) { u32 handle = nla_get_u32(tb[TCA_U32_LINK]); struct tc_u_hnode *ht_down = NULL, *ht_old; @@ -759,13 +764,9 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, tcf_bind_filter(tp, &n->res, base); } - if (tb[TCA_U32_INDEV]) { - int ret; - ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); - if (ret < 0) - return -EINVAL; - n->ifindex = ret; - } + if (ifindex >= 0) + n->ifindex = ifindex; + return 0; }
On Wed, May 31, 2023 at 10:16 AM Lee Jones <lee@kernel.org> wrote: > > In the event of a failure in tcf_change_indev(), u32_set_parms() will > immediately return without decrementing the recently incremented > reference counter. If this happens enough times, the counter will > rollover and the reference freed, leading to a double free which can be > used to do 'bad things'. > > Cc: stable@kernel.org # v4.14+ > Signed-off-by: Lee Jones <lee@kernel.org> > --- > net/sched/cls_u32.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c > index 4e2e269f121f8..fad61ca5e90bf 100644 > --- a/net/sched/cls_u32.c > +++ b/net/sched/cls_u32.c > @@ -762,8 +762,11 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, > if (tb[TCA_U32_INDEV]) { > int ret; > ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); > - if (ret < 0) > + if (ret < 0) { > + if (tb[TCA_U32_LINK]) > + n->ht_down->refcnt--; > return -EINVAL; > + } > n->ifindex = ret; > } > return 0; The spirit of the patch looks right I dont think this fully solves the issue you state. My suggestion: Move the if (tb[TCA_U32_INDEV]) above the if (tb[TCA_U32_LINK]) { Did you see this in practice or you found it by eyeballing the code? Can you also add a tdc test for it? There are simple ways to create the scenario. cheers, jamal > 2.41.0.rc0.172.g3f132b7071-goog >
On Wed, May 31, 2023 at 11:03 AM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, May 31, 2023 at 4:16 PM Lee Jones <lee@kernel.org> wrote: > > > > In the event of a failure in tcf_change_indev(), u32_set_parms() will > > immediately return without decrementing the recently incremented > > reference counter. If this happens enough times, the counter will > > rollover and the reference freed, leading to a double free which can be > > used to do 'bad things'. > > > > Cc: stable@kernel.org # v4.14+ > > Please add a Fixes: tag. > > > Signed-off-by: Lee Jones <lee@kernel.org> > > --- > > net/sched/cls_u32.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c > > index 4e2e269f121f8..fad61ca5e90bf 100644 > > --- a/net/sched/cls_u32.c > > +++ b/net/sched/cls_u32.c > > @@ -762,8 +762,11 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, > > if (tb[TCA_U32_INDEV]) { > > int ret; > > ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); > > This call should probably be done earlier in the function, next to > tcf_exts_validate_ex() > > Otherwise we might ask why the tcf_bind_filter() does not need to be undone. > > Something like: > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c > index 4e2e269f121f8a301368b9783753e055f5af6a4e..ac957ff2216ae18bcabdd3af3b0e127447ef8f91 > 100644 > --- a/net/sched/cls_u32.c > +++ b/net/sched/cls_u32.c > @@ -718,13 +718,18 @@ static int u32_set_parms(struct net *net, struct > tcf_proto *tp, > struct nlattr *est, u32 flags, u32 fl_flags, > struct netlink_ext_ack *extack) > { > - int err; > + int err, ifindex = -1; > > err = tcf_exts_validate_ex(net, tp, tb, est, &n->exts, flags, > fl_flags, extack); > if (err < 0) > return err; > > + if (tb[TCA_U32_INDEV]) { > + ifindex = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); > + if (ifindex < 0) > + return -EINVAL; > + } > if (tb[TCA_U32_LINK]) { > u32 handle = nla_get_u32(tb[TCA_U32_LINK]); > struct tc_u_hnode *ht_down = NULL, *ht_old; > @@ -759,13 +764,9 @@ static int u32_set_parms(struct net *net, struct > tcf_proto *tp, > tcf_bind_filter(tp, &n->res, base); > } > > - if (tb[TCA_U32_INDEV]) { > - int ret; > - ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); > - if (ret < 0) > - return -EINVAL; > - n->ifindex = ret; > - } > + if (ifindex >= 0) > + n->ifindex = ifindex; > + I guess we crossed paths ;-> Please, add a tdc test as well - it doesnt have to be in this patch, can be a followup. cheers, jamal > return 0; > }
On Wed, 31 May 2023, Jamal Hadi Salim wrote: > On Wed, May 31, 2023 at 11:03 AM Eric Dumazet <edumazet@google.com> wrote: > > > > On Wed, May 31, 2023 at 4:16 PM Lee Jones <lee@kernel.org> wrote: > > > > > > In the event of a failure in tcf_change_indev(), u32_set_parms() will > > > immediately return without decrementing the recently incremented > > > reference counter. If this happens enough times, the counter will > > > rollover and the reference freed, leading to a double free which can be > > > used to do 'bad things'. > > > > > > Cc: stable@kernel.org # v4.14+ > > > > Please add a Fixes: tag. Why? From memory, I couldn't identify a specific commit to fix, which is why I used a Cc tag as per the Stable documentation: Option 1 ******** To have the patch automatically included in the stable tree, add the tag .. code-block:: none Cc: stable@vger.kernel.org in the sign-off area. Once the patch is merged it will be applied to the stable tree without anything else needing to be done by the author or subsystem maintainer. > > > Signed-off-by: Lee Jones <lee@kernel.org> > > > --- > > > net/sched/cls_u32.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c > > > index 4e2e269f121f8..fad61ca5e90bf 100644 > > > --- a/net/sched/cls_u32.c > > > +++ b/net/sched/cls_u32.c > > > @@ -762,8 +762,11 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, > > > if (tb[TCA_U32_INDEV]) { > > > int ret; > > > ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); > > > > This call should probably be done earlier in the function, next to > > tcf_exts_validate_ex() > > > > Otherwise we might ask why the tcf_bind_filter() does not need to be undone. > > > > Something like: > > > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c > > index 4e2e269f121f8a301368b9783753e055f5af6a4e..ac957ff2216ae18bcabdd3af3b0e127447ef8f91 > > 100644 > > --- a/net/sched/cls_u32.c > > +++ b/net/sched/cls_u32.c > > @@ -718,13 +718,18 @@ static int u32_set_parms(struct net *net, struct > > tcf_proto *tp, > > struct nlattr *est, u32 flags, u32 fl_flags, > > struct netlink_ext_ack *extack) > > { > > - int err; > > + int err, ifindex = -1; > > > > err = tcf_exts_validate_ex(net, tp, tb, est, &n->exts, flags, > > fl_flags, extack); > > if (err < 0) > > return err; > > > > + if (tb[TCA_U32_INDEV]) { > > + ifindex = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); > > + if (ifindex < 0) > > + return -EINVAL; > > + } Thanks for the advice. Leave it with me. > > if (tb[TCA_U32_LINK]) { > > u32 handle = nla_get_u32(tb[TCA_U32_LINK]); > > struct tc_u_hnode *ht_down = NULL, *ht_old; > > @@ -759,13 +764,9 @@ static int u32_set_parms(struct net *net, struct > > tcf_proto *tp, > > tcf_bind_filter(tp, &n->res, base); > > } > > > > - if (tb[TCA_U32_INDEV]) { > > - int ret; > > - ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); > > - if (ret < 0) > > - return -EINVAL; > > - n->ifindex = ret; > > - } > > + if (ifindex >= 0) > > + n->ifindex = ifindex; > > + > > I guess we crossed paths ;-> > Please, add a tdc test as well - it doesnt have to be in this patch, > can be a followup. I don't know how to do that, or even what a 'tdc' is. Is it trivial? Can you point me towards the documentation please?
On Thu, Jun 1, 2023 at 4:06 PM Lee Jones <lee@kernel.org> wrote: > > On Wed, 31 May 2023, Jamal Hadi Salim wrote: > > > On Wed, May 31, 2023 at 11:03 AM Eric Dumazet <edumazet@google.com> wrote: > > > > > > On Wed, May 31, 2023 at 4:16 PM Lee Jones <lee@kernel.org> wrote: > > > > > > > > In the event of a failure in tcf_change_indev(), u32_set_parms() will > > > > immediately return without decrementing the recently incremented > > > > reference counter. If this happens enough times, the counter will > > > > rollover and the reference freed, leading to a double free which can be > > > > used to do 'bad things'. > > > > > > > > Cc: stable@kernel.org # v4.14+ > > > > > > Please add a Fixes: tag. > > Why? How have you identified v4.14+ ? Probably you did some research/"git archeology". By adding the Fixes: tag, you allow us to double check immediately, and see if other bugs need to be fixed at the same time. You can also CC blamed patch authors, to get some feedback. Otherwise, we (people reviewing this patch) have to also do this research from scratch. In this case, it seems bug was added in commit 705c7091262d02b09eb686c24491de61bf42fdb2 Author: Jiri Pirko <jiri@resnulli.us> Date: Fri Aug 4 14:29:14 2017 +0200 net: sched: cls_u32: no need to call tcf_exts_change for newly allocated struct A nice Fixes: tag would then be Fixes: 705c7091262d ("net: sched: cls_u32: no need to call tcf_exts_change for newly allocated struct") Thanks.
On Thu, 01 Jun 2023, Eric Dumazet wrote: > On Thu, Jun 1, 2023 at 4:06 PM Lee Jones <lee@kernel.org> wrote: > > > > On Wed, 31 May 2023, Jamal Hadi Salim wrote: > > > > > On Wed, May 31, 2023 at 11:03 AM Eric Dumazet <edumazet@google.com> wrote: > > > > > > > > On Wed, May 31, 2023 at 4:16 PM Lee Jones <lee@kernel.org> wrote: > > > > > > > > > > In the event of a failure in tcf_change_indev(), u32_set_parms() will > > > > > immediately return without decrementing the recently incremented > > > > > reference counter. If this happens enough times, the counter will > > > > > rollover and the reference freed, leading to a double free which can be > > > > > used to do 'bad things'. > > > > > > > > > > Cc: stable@kernel.org # v4.14+ > > > > > > > > Please add a Fixes: tag. > > > > Why? > > How have you identified v4.14+ ? > > Probably you did some research/"git archeology". > > By adding the Fixes: tag, you allow us to double check immediately, > and see if other bugs need to be fixed at the same time. > > You can also CC blamed patch authors, to get some feedback. > > Otherwise, we (people reviewing this patch) have to also do this > research from scratch. > > In this case, it seems bug was added in > > commit 705c7091262d02b09eb686c24491de61bf42fdb2 > Author: Jiri Pirko <jiri@resnulli.us> > Date: Fri Aug 4 14:29:14 2017 +0200 > > net: sched: cls_u32: no need to call tcf_exts_change for newly > allocated struct > > > A nice Fixes: tag would then be > > Fixes: 705c7091262d ("net: sched: cls_u32: no need to call > tcf_exts_change for newly allocated struct") Thanks for digging this out. I will add it.
On Thu, Jun 1, 2023 at 10:06 AM Lee Jones <lee@kernel.org> wrote: > > On Wed, 31 May 2023, Jamal Hadi Salim wrote: > > > On Wed, May 31, 2023 at 11:03 AM Eric Dumazet <edumazet@google.com> wrote: > > > > > > On Wed, May 31, 2023 at 4:16 PM Lee Jones <lee@kernel.org> wrote: > > > > > > > > In the event of a failure in tcf_change_indev(), u32_set_parms() will > > > > immediately return without decrementing the recently incremented > > > > reference counter. If this happens enough times, the counter will > > > > rollover and the reference freed, leading to a double free which can be > > > > used to do 'bad things'. > > > > > > > > Cc: stable@kernel.org # v4.14+ > > > > > > Please add a Fixes: tag. > > Why? > > From memory, I couldn't identify a specific commit to fix, which is why > I used a Cc tag as per the Stable documentation: > > Option 1 > ******** > > To have the patch automatically included in the stable tree, add the tag > > .. code-block:: none > > Cc: stable@vger.kernel.org > > in the sign-off area. Once the patch is merged it will be applied to > the stable tree without anything else needing to be done by the author > or subsystem maintainer. > > > > > Signed-off-by: Lee Jones <lee@kernel.org> > > > > --- > > > > net/sched/cls_u32.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c > > > > index 4e2e269f121f8..fad61ca5e90bf 100644 > > > > --- a/net/sched/cls_u32.c > > > > +++ b/net/sched/cls_u32.c > > > > @@ -762,8 +762,11 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, > > > > if (tb[TCA_U32_INDEV]) { > > > > int ret; > > > > ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); > > > > > > This call should probably be done earlier in the function, next to > > > tcf_exts_validate_ex() > > > > > > Otherwise we might ask why the tcf_bind_filter() does not need to be undone. > > > > > > Something like: > > > > > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c > > > index 4e2e269f121f8a301368b9783753e055f5af6a4e..ac957ff2216ae18bcabdd3af3b0e127447ef8f91 > > > 100644 > > > --- a/net/sched/cls_u32.c > > > +++ b/net/sched/cls_u32.c > > > @@ -718,13 +718,18 @@ static int u32_set_parms(struct net *net, struct > > > tcf_proto *tp, > > > struct nlattr *est, u32 flags, u32 fl_flags, > > > struct netlink_ext_ack *extack) > > > { > > > - int err; > > > + int err, ifindex = -1; > > > > > > err = tcf_exts_validate_ex(net, tp, tb, est, &n->exts, flags, > > > fl_flags, extack); > > > if (err < 0) > > > return err; > > > > > > + if (tb[TCA_U32_INDEV]) { > > > + ifindex = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); > > > + if (ifindex < 0) > > > + return -EINVAL; > > > + } > > Thanks for the advice. Leave it with me. > > > > if (tb[TCA_U32_LINK]) { > > > u32 handle = nla_get_u32(tb[TCA_U32_LINK]); > > > struct tc_u_hnode *ht_down = NULL, *ht_old; > > > @@ -759,13 +764,9 @@ static int u32_set_parms(struct net *net, struct > > > tcf_proto *tp, > > > tcf_bind_filter(tp, &n->res, base); > > > } > > > > > > - if (tb[TCA_U32_INDEV]) { > > > - int ret; > > > - ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); > > > - if (ret < 0) > > > - return -EINVAL; > > > - n->ifindex = ret; > > > - } > > > + if (ifindex >= 0) > > > + n->ifindex = ifindex; > > > + > > > > I guess we crossed paths ;-> > > > Please, add a tdc test as well - it doesnt have to be in this patch, > > can be a followup. > > I don't know how to do that, or even what a 'tdc' is. Is it trivial? > > Can you point me towards the documentation please? There is a README in tools/testing/selftests/tc-testing/README If you created the scenario by running some tc command line it should not be difficult to create such a test. Or just tell us what command line you used to create it and we can help do one for you this time. If you found the issue by just eyeballing the code or syzkaller then just say that in the commit. cheers, jamal > -- > Lee Jones [李琼斯]
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c index 4e2e269f121f8..fad61ca5e90bf 100644 --- a/net/sched/cls_u32.c +++ b/net/sched/cls_u32.c @@ -762,8 +762,11 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, if (tb[TCA_U32_INDEV]) { int ret; ret = tcf_change_indev(net, tb[TCA_U32_INDEV], extack); - if (ret < 0) + if (ret < 0) { + if (tb[TCA_U32_LINK]) + n->ht_down->refcnt--; return -EINVAL; + } n->ifindex = ret; } return 0;