[v1] net/sched: Prevent notify to parent who unsupport class ops

[PATCH] net/sched: Prevent notify to parent who unsupport class ops

Posted by Lizhi Xu 3 months ago

If the parent qdisc does not support class operations then exit notify.

In addition, the validity of the cl value is judged before executing the
notify. Similarly, the notify is exited when the address represented by
its value is invalid.

Reported-by: syzbot+1261670bbdefc5485a06@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1261670bbdefc5485a06
Tested-by: syzbot+1261670bbdefc5485a06@syzkaller.appspotmail.com
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 net/sched/sch_api.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index d8a33486c51..53fd63af14d 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -803,12 +803,13 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, int n, int len)
 			break;
 		}
 		cops = sch->ops->cl_ops;
-		if (notify && cops->qlen_notify) {
+		if (cops && notify && cops->qlen_notify) {
 			/* Note that qlen_notify must be idempotent as it may get called
 			 * multiple times.
 			 */
 			cl = cops->find(sch, parentid);
-			cops->qlen_notify(sch, cl);
+			if (virt_addr_valid(cl))
+				cops->qlen_notify(sch, cl);
 		}
 		sch->q.qlen -= n;
 		sch->qstats.backlog -= len;
-- 
2.43.0

Re: [PATCH] net/sched: Prevent notify to parent who unsupport class ops

Posted by Cong Wang 3 months ago

(Cc Victor)

On Fri, Jul 04, 2025 at 04:04:21PM +0800, Lizhi Xu wrote:
> If the parent qdisc does not support class operations then exit notify.
> 
> In addition, the validity of the cl value is judged before executing the
> notify. Similarly, the notify is exited when the address represented by
> its value is invalid.
> 
> Reported-by: syzbot+1261670bbdefc5485a06@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=1261670bbdefc5485a06
> Tested-by: syzbot+1261670bbdefc5485a06@syzkaller.appspotmail.com
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>

Victor also posted a fix here:
https://lore.kernel.org/netdev/20250704163422.160424-1-victor@mojatatu.com/

I asked Victor there if we still need to patch
qdisc_tree_reduce_backlog().

> ---
>  net/sched/sch_api.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index d8a33486c51..53fd63af14d 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -803,12 +803,13 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, int n, int len)
>  			break;
>  		}
>  		cops = sch->ops->cl_ops;
> -		if (notify && cops->qlen_notify) {
> +		if (cops && notify && cops->qlen_notify) {

I think 'notify' should be tested first, as it was.

>  			/* Note that qlen_notify must be idempotent as it may get called
>  			 * multiple times.
>  			 */
>  			cl = cops->find(sch, parentid);
> -			cops->qlen_notify(sch, cl);
> +			if (virt_addr_valid(cl))

This is not how we test NULL or error pointers. Just "if (cl)" should
be sufficient for NULL case.

Thanks.

[PATCH V2] net/sched: Prevent notify to parent who unsupport class ops

Posted by Lizhi Xu 3 months ago

If the parent qdisc does not support class operations then exit notify.

In addition, the validity of the cl value is judged before executing the
notify. Similarly, the notify is exited when the address represented by
its value is invalid.

Reported-by: syzbot+1261670bbdefc5485a06@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1261670bbdefc5485a06
Tested-by: syzbot+1261670bbdefc5485a06@syzkaller.appspotmail.com
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
V1 -> V2: movie notify check first and check cl NULL

 net/sched/sch_api.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index d8a33486c51..53fd63af14d 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -803,12 +803,13 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, int n, int len)
 			break;
 		}
 		cops = sch->ops->cl_ops;
-		if (notify && cops->qlen_notify) {
+		if (notify && cops && cops->qlen_notify) {
 			/* Note that qlen_notify must be idempotent as it may get called
 			 * multiple times.
 			 */
 			cl = cops->find(sch, parentid);
-			cops->qlen_notify(sch, cl);
+			if (cl)
+				cops->qlen_notify(sch, cl);
 		}
 		sch->q.qlen -= n;
 		sch->qstats.backlog -= len;
-- 
2.43.0

Re: [PATCH V2] net/sched: Prevent notify to parent who unsupport class ops

Posted by Cong Wang 3 months ago

Hi Lizhi,

On Sat, Jul 05, 2025 at 09:18:22AM +0800, Lizhi Xu wrote:
> If the parent qdisc does not support class operations then exit notify.
> 
> In addition, the validity of the cl value is judged before executing the
> notify. Similarly, the notify is exited when the address represented by
> its value is invalid.
> 
> Reported-by: syzbot+1261670bbdefc5485a06@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=1261670bbdefc5485a06

Maybe I didn't make it clear, I think Victor's patch also fixes this
bug.

https://lore.kernel.org/netdev/20250704163422.160424-1-victor@mojatatu.com/

Can you check if you still see the crash with his fix?

The reason why I am asking is because his fix addresses a problem
earlier on the code path, which possibly makes your fix unnecessary.
Hence, his fix is closer to the root cause.

Please test and confirm.

Thanks!

Re: [PATCH V2] net/sched: Prevent notify to parent who unsupport class ops

Posted by Jamal Hadi Salim 3 months ago

On Sat, Jul 5, 2025 at 1:07 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> Hi Lizhi,
>
> On Sat, Jul 05, 2025 at 09:18:22AM +0800, Lizhi Xu wrote:
> > If the parent qdisc does not support class operations then exit notify.
> >
> > In addition, the validity of the cl value is judged before executing the
> > notify. Similarly, the notify is exited when the address represented by
> > its value is invalid.
> >
> > Reported-by: syzbot+1261670bbdefc5485a06@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=1261670bbdefc5485a06
>
> Maybe I didn't make it clear, I think Victor's patch also fixes this
> bug.
>
> https://lore.kernel.org/netdev/20250704163422.160424-1-victor@mojatatu.com/
>
> Can you check if you still see the crash with his fix?
>

syzbot+1261670bbdefc5485a06@syzkaller.appspotmail.com (in this report)
is part of what Victor's patch fixes (as listed in that patch).

cheers,
jamal

> The reason why I am asking is because his fix addresses a problem
> earlier on the code path, which possibly makes your fix unnecessary.
> Hence, his fix is closer to the root cause.
>
> Please test and confirm.
>

V
> Thanks!

Re: [PATCH] net/sched: Prevent notify to parent who unsupport class ops

Posted by Paolo Abeni 3 months ago

On 7/4/25 10:04 AM, Lizhi Xu wrote:
> If the parent qdisc does not support class operations then exit notify.
> 
> In addition, the validity of the cl value is judged before executing the
> notify. Similarly, the notify is exited when the address represented by
> its value is invalid.
> 
> Reported-by: syzbot+1261670bbdefc5485a06@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=1261670bbdefc5485a06
> Tested-by: syzbot+1261670bbdefc5485a06@syzkaller.appspotmail.com
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
>  net/sched/sch_api.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index d8a33486c51..53fd63af14d 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -803,12 +803,13 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, int n, int len)
>  			break;
>  		}
>  		cops = sch->ops->cl_ops;
> -		if (notify && cops->qlen_notify) {
> +		if (cops && notify && cops->qlen_notify) {
>  			/* Note that qlen_notify must be idempotent as it may get called
>  			 * multiple times.
>  			 */
>  			cl = cops->find(sch, parentid);
> -			cops->qlen_notify(sch, cl);
> +			if (virt_addr_valid(cl))
> +				cops->qlen_notify(sch, cl);

The above causes build failures in arm64 builds:

  ../net/sched/sch_api.c: In function ‘qdisc_tree_reduce_backlog’:
  ../arch/arm64/include/asm/memory.h:424:66: error: passing argument 1
of ‘virt_to_pfn’ makes pointer from integer without a cast
[-Wint-conversion]
    424 |         __is_lm_address(__addr) &&
pfn_is_map_memory(virt_to_pfn(__addr));      \

/P