All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATH net] net/sched: ets: fix crash when flipping from 'strict' to 'quantum'
@ 2021-08-24 22:33 Davide Caratti
  2021-08-25 10:20 ` patchwork-bot+netdevbpf
  2021-08-31  0:43 ` Cong Wang
  0 siblings, 2 replies; 7+ messages in thread
From: Davide Caratti @ 2021-08-24 22:33 UTC (permalink / raw)
  To: Hangbin Liu, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
	David S. Miller, Jakub Kicinski, Petr Machata, netdev

While running kselftests, Hangbin observed that sch_ets.sh often crashes,
and splats like the following one are seen in the output of 'dmesg':

 BUG: kernel NULL pointer dereference, address: 0000000000000000
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x0000) - not-present page
 PGD 159f12067 P4D 159f12067 PUD 159f13067 PMD 0
 Oops: 0000 [#1] SMP NOPTI
 CPU: 2 PID: 921 Comm: tc Not tainted 5.14.0-rc6+ #458
 Hardware name: Red Hat KVM, BIOS 1.11.1-4.module+el8.1.0+4066+0f1aadab 04/01/2014
 RIP: 0010:__list_del_entry_valid+0x2d/0x50
 Code: 48 8b 57 08 48 b9 00 01 00 00 00 00 ad de 48 39 c8 0f 84 ac 6e 5b 00 48 b9 22 01 00 00 00 00 ad de 48 39 ca 0f 84 cf 6e 5b 00 <48> 8b 32 48 39 fe 0f 85 af 6e 5b 00 48 8b 50 08 48 39 f2 0f 85 94
 RSP: 0018:ffffb2da005c3890 EFLAGS: 00010217
 RAX: 0000000000000000 RBX: ffff9073ba23f800 RCX: dead000000000122
 RDX: 0000000000000000 RSI: 0000000000000008 RDI: ffff9073ba23fbc8
 RBP: ffff9073ba23f890 R08: 0000000000000001 R09: 0000000000000001
 R10: 0000000000000001 R11: 0000000000000001 R12: dead000000000100
 R13: ffff9073ba23fb00 R14: 0000000000000002 R15: 0000000000000002
 FS:  00007f93e5564e40(0000) GS:ffff9073bba00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 0000000000000000 CR3: 000000014ad34000 CR4: 0000000000350ee0
 Call Trace:
  ets_qdisc_reset+0x6e/0x100 [sch_ets]
  qdisc_reset+0x49/0x1d0
  tbf_reset+0x15/0x60 [sch_tbf]
  qdisc_reset+0x49/0x1d0
  dev_reset_queue.constprop.42+0x2f/0x90
  dev_deactivate_many+0x1d3/0x3d0
  dev_deactivate+0x56/0x90
  qdisc_graft+0x47e/0x5a0
  tc_get_qdisc+0x1db/0x3e0
  rtnetlink_rcv_msg+0x164/0x4c0
  netlink_rcv_skb+0x50/0x100
  netlink_unicast+0x1a5/0x280
  netlink_sendmsg+0x242/0x480
  sock_sendmsg+0x5b/0x60
  ____sys_sendmsg+0x1f2/0x260
  ___sys_sendmsg+0x7c/0xc0
  __sys_sendmsg+0x57/0xa0
  do_syscall_64+0x3a/0x80
  entry_SYSCALL_64_after_hwframe+0x44/0xae
 RIP: 0033:0x7f93e44b8338
 Code: 89 02 48 c7 c0 ff ff ff ff eb b5 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 8d 05 25 43 2c 00 8b 00 85 c0 75 17 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 54 41 89 d4 55
 RSP: 002b:00007ffc0db737a8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
 RAX: ffffffffffffffda RBX: 0000000061255c06 RCX: 00007f93e44b8338
 RDX: 0000000000000000 RSI: 00007ffc0db73810 RDI: 0000000000000003
 RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
 R10: 000000000000000b R11: 0000000000000246 R12: 0000000000000001
 R13: 0000000000687880 R14: 0000000000000000 R15: 0000000000000000
 Modules linked in: sch_ets sch_tbf dummy rfkill iTCO_wdt iTCO_vendor_support intel_rapl_msr intel_rapl_common joydev i2c_i801 pcspkr i2c_smbus lpc_ich virtio_balloon ip_tables xfs libcrc32c crct10dif_pclmul crc32_pclmul crc32c_intel ahci libahci ghash_clmulni_intel libata serio_raw virtio_blk virtio_console virtio_net net_failover failover sunrpc dm_mirror dm_region_hash dm_log dm_mod
 CR2: 0000000000000000

When the change() function decreases the value of 'nstrict', we must take
into account that packets might be already enqueued on a class that flips
from 'strict' to 'quantum': otherwise that class will not be added to the
bandwidth-sharing list. Then, a call to ets_qdisc_reset() will attempt to
do list_del(&alist) with 'alist' filled with zero, hence the NULL pointer
dereference.
For classes flipping from 'strict' to 'quantum', initialize an empty list
and eventually add it to the bandwidth-sharing list, if there are packets
already enqueued. In this way, the kernel will:
 a) prevent crashing as described above.
 b) avoid retaining the backlog packets (for an arbitrarily long time) in
    case no packet is enqueued after a change from 'strict' to 'quantum'.

Reported-by: Hangbin Liu <liuhangbin@gmail.com>
Fixes: dcc68b4d8084 ("net: sch_ets: Add a new Qdisc")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/sched/sch_ets.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
index c1e84d1eeaba..c76701ac35ab 100644
--- a/net/sched/sch_ets.c
+++ b/net/sched/sch_ets.c
@@ -660,6 +660,13 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
 	sch_tree_lock(sch);
 
 	q->nbands = nbands;
+	for (i = nstrict; i < q->nstrict; i++) {
+		INIT_LIST_HEAD(&q->classes[i].alist);
+		if (q->classes[i].qdisc->q.qlen) {
+			list_add_tail(&q->classes[i].alist, &q->active);
+			q->classes[i].deficit = quanta[i];
+		}
+	}
 	q->nstrict = nstrict;
 	memcpy(q->prio2band, priomap, sizeof(priomap));
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATH net] net/sched: ets: fix crash when flipping from 'strict' to 'quantum'
  2021-08-24 22:33 [PATH net] net/sched: ets: fix crash when flipping from 'strict' to 'quantum' Davide Caratti
@ 2021-08-25 10:20 ` patchwork-bot+netdevbpf
  2021-08-31  0:43 ` Cong Wang
  1 sibling, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-08-25 10:20 UTC (permalink / raw)
  To: Davide Caratti
  Cc: liuhangbin, jhs, xiyou.wangcong, jiri, davem, kuba, petrm, netdev

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Wed, 25 Aug 2021 00:33:48 +0200 you wrote:
> While running kselftests, Hangbin observed that sch_ets.sh often crashes,
> and splats like the following one are seen in the output of 'dmesg':
> 
>  BUG: kernel NULL pointer dereference, address: 0000000000000000
>  #PF: supervisor read access in kernel mode
>  #PF: error_code(0x0000) - not-present page
>  PGD 159f12067 P4D 159f12067 PUD 159f13067 PMD 0
>  Oops: 0000 [#1] SMP NOPTI
>  CPU: 2 PID: 921 Comm: tc Not tainted 5.14.0-rc6+ #458
>  Hardware name: Red Hat KVM, BIOS 1.11.1-4.module+el8.1.0+4066+0f1aadab 04/01/2014
>  RIP: 0010:__list_del_entry_valid+0x2d/0x50
>  Code: 48 8b 57 08 48 b9 00 01 00 00 00 00 ad de 48 39 c8 0f 84 ac 6e 5b 00 48 b9 22 01 00 00 00 00 ad de 48 39 ca 0f 84 cf 6e 5b 00 <48> 8b 32 48 39 fe 0f 85 af 6e 5b 00 48 8b 50 08 48 39 f2 0f 85 94
>  RSP: 0018:ffffb2da005c3890 EFLAGS: 00010217
>  RAX: 0000000000000000 RBX: ffff9073ba23f800 RCX: dead000000000122
>  RDX: 0000000000000000 RSI: 0000000000000008 RDI: ffff9073ba23fbc8
>  RBP: ffff9073ba23f890 R08: 0000000000000001 R09: 0000000000000001
>  R10: 0000000000000001 R11: 0000000000000001 R12: dead000000000100
>  R13: ffff9073ba23fb00 R14: 0000000000000002 R15: 0000000000000002
>  FS:  00007f93e5564e40(0000) GS:ffff9073bba00000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 0000000000000000 CR3: 000000014ad34000 CR4: 0000000000350ee0
>  Call Trace:
>   ets_qdisc_reset+0x6e/0x100 [sch_ets]
>   qdisc_reset+0x49/0x1d0
>   tbf_reset+0x15/0x60 [sch_tbf]
>   qdisc_reset+0x49/0x1d0
>   dev_reset_queue.constprop.42+0x2f/0x90
>   dev_deactivate_many+0x1d3/0x3d0
>   dev_deactivate+0x56/0x90
>   qdisc_graft+0x47e/0x5a0
>   tc_get_qdisc+0x1db/0x3e0
>   rtnetlink_rcv_msg+0x164/0x4c0
>   netlink_rcv_skb+0x50/0x100
>   netlink_unicast+0x1a5/0x280
>   netlink_sendmsg+0x242/0x480
>   sock_sendmsg+0x5b/0x60
>   ____sys_sendmsg+0x1f2/0x260
>   ___sys_sendmsg+0x7c/0xc0
>   __sys_sendmsg+0x57/0xa0
>   do_syscall_64+0x3a/0x80
>   entry_SYSCALL_64_after_hwframe+0x44/0xae
>  RIP: 0033:0x7f93e44b8338
>  Code: 89 02 48 c7 c0 ff ff ff ff eb b5 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 8d 05 25 43 2c 00 8b 00 85 c0 75 17 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 54 41 89 d4 55
>  RSP: 002b:00007ffc0db737a8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
>  RAX: ffffffffffffffda RBX: 0000000061255c06 RCX: 00007f93e44b8338
>  RDX: 0000000000000000 RSI: 00007ffc0db73810 RDI: 0000000000000003
>  RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
>  R10: 000000000000000b R11: 0000000000000246 R12: 0000000000000001
>  R13: 0000000000687880 R14: 0000000000000000 R15: 0000000000000000
>  Modules linked in: sch_ets sch_tbf dummy rfkill iTCO_wdt iTCO_vendor_support intel_rapl_msr intel_rapl_common joydev i2c_i801 pcspkr i2c_smbus lpc_ich virtio_balloon ip_tables xfs libcrc32c crct10dif_pclmul crc32_pclmul crc32c_intel ahci libahci ghash_clmulni_intel libata serio_raw virtio_blk virtio_console virtio_net net_failover failover sunrpc dm_mirror dm_region_hash dm_log dm_mod
>  CR2: 0000000000000000
> 
> [...]

Here is the summary with links:
  - [PATH,net] net/sched: ets: fix crash when flipping from 'strict' to 'quantum'
    https://git.kernel.org/netdev/net/c/cd9b50adc6bb

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATH net] net/sched: ets: fix crash when flipping from 'strict' to 'quantum'
  2021-08-24 22:33 [PATH net] net/sched: ets: fix crash when flipping from 'strict' to 'quantum' Davide Caratti
  2021-08-25 10:20 ` patchwork-bot+netdevbpf
@ 2021-08-31  0:43 ` Cong Wang
  2021-08-31  9:54   ` Davide Caratti
  1 sibling, 1 reply; 7+ messages in thread
From: Cong Wang @ 2021-08-31  0:43 UTC (permalink / raw)
  To: Davide Caratti
  Cc: Hangbin Liu, Jamal Hadi Salim, Jiri Pirko, David S. Miller,
	Jakub Kicinski, Petr Machata, Linux Kernel Network Developers

On Tue, Aug 24, 2021 at 3:34 PM Davide Caratti <dcaratti@redhat.com> wrote:
> When the change() function decreases the value of 'nstrict', we must take
> into account that packets might be already enqueued on a class that flips
> from 'strict' to 'quantum': otherwise that class will not be added to the
> bandwidth-sharing list. Then, a call to ets_qdisc_reset() will attempt to
> do list_del(&alist) with 'alist' filled with zero, hence the NULL pointer
> dereference.

I am confused about how we end up having NULL in list head.

From your changelog, you imply it happens when we change an existing
Qdisc, but I don't see any place that could set this list head to NULL.
list_del() clearly doesn't set NULL.

But if it is a new Qdisc, Qdisc is allocated with zero's hence having NULL
as list head. However, in this case, q->nstrict should be 0 before the loop,
so I don't think your code helps at all as the for loop can't even be entered?

Thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATH net] net/sched: ets: fix crash when flipping from 'strict' to 'quantum'
  2021-08-31  0:43 ` Cong Wang
@ 2021-08-31  9:54   ` Davide Caratti
  2021-08-31 18:16     ` Cong Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Davide Caratti @ 2021-08-31  9:54 UTC (permalink / raw)
  To: Linux Kernel Network Developers
  Cc: xiyou.wangcong, davem, jhs, jiri, kuba, liuhangbin, petrm

hello Cong, thanks a lot for looking at this!

On Mon, Aug 30, 2021 at 05:43:09PM -0700, Cong Wang wrote:
> On Tue, Aug 24, 2021 at 3:34 PM Davide Caratti <dcaratti@redhat.com> wrote:
> > When the change() function decreases the value of 'nstrict', we must take
> > into account that packets might be already enqueued on a class that flips
> > from 'strict' to 'quantum': otherwise that class will not be added to the
> > bandwidth-sharing list. Then, a call to ets_qdisc_reset() will attempt to
> > do list_del(&alist) with 'alist' filled with zero, hence the NULL pointer
> > dereference.
> 
> I am confused about how we end up having NULL in list head.
> 
> From your changelog, you imply it happens when we change an existing
> Qdisc, but I don't see any place that could set this list head to NULL.
> list_del() clearly doesn't set NULL.

right, the problem happens when we try to *decrease* the value of 'nstrict'
while traffic is being loaded.

ETS classes from 0 to nstrict - 1 don't initialize this list at all, nor they
use it. The initialization happens when the first packet is enqueued to one of
the bandwidth-sharing classes (from nstrict to nbands - 1), see [1]).

However, if we modify the value of 'nstrict' while q->classes[i].qdisc->q.len is
greater than zero, the list initialization is probably not going to happen, so
the struct

q->classes[i].alist

remains filled with zero even since the first allocation, like you mentioned below:

> But if it is a new Qdisc, Qdisc is allocated with zero's hence having NULL
> as list head. However, in this case, q->nstrict should be 0 before the loop,

^^ this. But you are wrong, q->nstrict can be any value from 0 to 16 (inclusive)
before the loop. As the value of 'nstrict' is reduced (e.g. from 4 to 0), the code
can reach the loop in ets_qdisc_change() with the following 4 conditions
simultaneously true:

1) nstrict = 0
2) q->nstrict = 4
3) q->classes[2].qdisc->q.qlen  greater than zero
4) q->classes[2].alist filled with 0

then, the value of q->nstrict is updated to 0. After that, ets_qdisc_reset() can be
called on unpatched Linux with the following 3 conditions simultaneously true:

a) q->nstrict = 0
b) q->classes[2].qdisc->q.qlen greater than zero
c) q->classes[2].alist filled with 0

that leads to the reported crash.

> so I don't think your code helps at all as the for loop can't even be entered?

The first code I tried was just doing INIT_LIST_HEAD(&q->classes[i].alist), with 
i ranging from nstrict to q->nstrict - 1: it fixed the crash in ets_qdisc_reset().

But then I observed that classes changing from strict to quantum were not sharing
any bandwidth at all, and they had a non-empty backlog even after I stopped all
the traffic: so, I added the 

	if (q->classes[i].qdisc->q.qlen) {
		list_add_tail(&q->classes[i].alist, &q->active);
		q->classes[i].deficit = quanta[i];
	}

part, that updates the DRR list with non-empty classes that are changing from
strict to DRR. After that, I verified that all classes were depleted correctly.

On a second thought, this INIT_LIST_HEAD(&q->classes[i].alist) line is no more
useful. If q->classes[i].qdisc->q.qlen is 0, either it remains 0 until the call
to ets_qdisc_reset(), or some packet is enqueued after q->nstrict is updated,
and the enqueueing of a 'first' packet [1] will fix the value of
q->classes[i].alist.
Finally, if q->classes[i].qdisc->q.qlen is bigger than zero, the list_add_tail()
part of my patch covers the missing update of the DRR list. In all these cases,
the NULL dereference in ets_qdisc_reset() is prevented.

So, I can probably send a patch (for net-next, when it reopens) that removes
this INIT_LIST_HEAD() line; anyway, its presence is harmless IMO. WDYT?

> 
> Thanks.

BTW, please find below a reproducer that's more efficient than kselftests in
seeing the problem:

    1   #!/bin/bash
    2   ip link del dev ddd0 >/dev/null 2>&1
    3   ip link add dev ddd0 type dummy
    4   ip link set dev ddd0 up
    5   tc qdisc add dev ddd0 handle 1: root tbf rate 100kbit latency 1000ms burst 1Mbit
    6   tc qdisc add dev ddd0 handle 10: parent 1: ets bands 4 strict 4 priomap  2 2 2 2 2 2 2 2 2 2 2 2 2 2 2 2
    7   tc qdisc show dev ddd0
    8   mausezahn ddd0 -A 10.10.10.1 -B 10.10.10.2 -c 0 -a own -b 00:c1:a0:c1:a0:00 -t udp &
    9   mpid=$!
   10   sleep 5
   11   tc qdisc change dev ddd0 handle 10: ets bands 8 quanta 2500 2500 2500 2500 2500 2500 2500 2500  priomap 2 2 2 2 2 2 2 2 2 2 2 2 2 2 2 2
   12   tc qdisc del dev ddd0 root
   13   sleep 1
   14   tc qdisc show dev ddd0
   15   kill $mpid
   16   rmmod sch_ets

Line 11 changes the ets qdisc from 4 prio to 8 DRR, so the value of q->nstrict
changes from 4 to 0. The crash in ets_qdisc_reset() happens with band number #2
(that's the only class being loaded with traffic during the test).

thanks!

-- 
davide

[1] https://elixir.bootlin.com/linux/v5.14/source/net/sched/sch_ets.c#L445


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATH net] net/sched: ets: fix crash when flipping from 'strict' to 'quantum'
  2021-08-31  9:54   ` Davide Caratti
@ 2021-08-31 18:16     ` Cong Wang
  2021-09-01 20:53       ` Davide Caratti
  0 siblings, 1 reply; 7+ messages in thread
From: Cong Wang @ 2021-08-31 18:16 UTC (permalink / raw)
  To: Davide Caratti
  Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
	Jiri Pirko, Jakub Kicinski, Hangbin Liu, Petr Machata

On Tue, Aug 31, 2021 at 2:54 AM Davide Caratti <dcaratti@redhat.com> wrote:
>
> hello Cong, thanks a lot for looking at this!
>
> On Mon, Aug 30, 2021 at 05:43:09PM -0700, Cong Wang wrote:
> > On Tue, Aug 24, 2021 at 3:34 PM Davide Caratti <dcaratti@redhat.com> wrote:
> > > When the change() function decreases the value of 'nstrict', we must take
> > > into account that packets might be already enqueued on a class that flips
> > > from 'strict' to 'quantum': otherwise that class will not be added to the
> > > bandwidth-sharing list. Then, a call to ets_qdisc_reset() will attempt to
> > > do list_del(&alist) with 'alist' filled with zero, hence the NULL pointer
> > > dereference.
> >
> > I am confused about how we end up having NULL in list head.
> >
> > From your changelog, you imply it happens when we change an existing
> > Qdisc, but I don't see any place that could set this list head to NULL.
> > list_del() clearly doesn't set NULL.
>
> right, the problem happens when we try to *decrease* the value of 'nstrict'
> while traffic is being loaded.
>
> ETS classes from 0 to nstrict - 1 don't initialize this list at all, nor they
> use it. The initialization happens when the first packet is enqueued to one of
> the bandwidth-sharing classes (from nstrict to nbands - 1), see [1]).
>
> However, if we modify the value of 'nstrict' while q->classes[i].qdisc->q.len is
> greater than zero, the list initialization is probably not going to happen, so
> the struct
>
> q->classes[i].alist
>
> remains filled with zero even since the first allocation, like you mentioned below:
>
> > But if it is a new Qdisc, Qdisc is allocated with zero's hence having NULL
> > as list head. However, in this case, q->nstrict should be 0 before the loop,
>
> ^^ this. But you are wrong, q->nstrict can be any value from 0 to 16 (inclusive)
> before the loop. As the value of 'nstrict' is reduced (e.g. from 4 to 0), the code
> can reach the loop in ets_qdisc_change() with the following 4 conditions
> simultaneously true:
>
> 1) nstrict = 0
> 2) q->nstrict = 4
> 3) q->classes[2].qdisc->q.qlen  greater than zero
> 4) q->classes[2].alist filled with 0
>
> then, the value of q->nstrict is updated to 0. After that, ets_qdisc_reset() can be
> called on unpatched Linux with the following 3 conditions simultaneously true:
>
> a) q->nstrict = 0
> b) q->classes[2].qdisc->q.qlen greater than zero
> c) q->classes[2].alist filled with 0
>
> that leads to the reported crash.
>
> > so I don't think your code helps at all as the for loop can't even be entered?
>
> The first code I tried was just doing INIT_LIST_HEAD(&q->classes[i].alist), with
> i ranging from nstrict to q->nstrict - 1: it fixed the crash in ets_qdisc_reset().
>
> But then I observed that classes changing from strict to quantum were not sharing
> any bandwidth at all, and they had a non-empty backlog even after I stopped all
> the traffic: so, I added the
>
>         if (q->classes[i].qdisc->q.qlen) {
>                 list_add_tail(&q->classes[i].alist, &q->active);
>                 q->classes[i].deficit = quanta[i];
>         }
>
> part, that updates the DRR list with non-empty classes that are changing from
> strict to DRR. After that, I verified that all classes were depleted correctly.
>
> On a second thought, this INIT_LIST_HEAD(&q->classes[i].alist) line is no more
> useful. If q->classes[i].qdisc->q.qlen is 0, either it remains 0 until the call
> to ets_qdisc_reset(), or some packet is enqueued after q->nstrict is updated,
> and the enqueueing of a 'first' packet [1] will fix the value of
> q->classes[i].alist.
> Finally, if q->classes[i].qdisc->q.qlen is bigger than zero, the list_add_tail()
> part of my patch covers the missing update of the DRR list. In all these cases,
> the NULL dereference in ets_qdisc_reset() is prevented.
>
> So, I can probably send a patch (for net-next, when it reopens) that removes
> this INIT_LIST_HEAD() line; anyway, its presence is harmless IMO. WDYT?

Actually I am thinking about the opposite, that is, always initializing the
list head. ;) Unless we always use list_add*() before list_del(), initializing
it unconditionally is a correct fix.

I do not doubt your patch works, however I worry that there might be
some other call paths which could call list_del() with the NULL.

Thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATH net] net/sched: ets: fix crash when flipping from 'strict' to 'quantum'
  2021-08-31 18:16     ` Cong Wang
@ 2021-09-01 20:53       ` Davide Caratti
  2021-09-03 23:50         ` Cong Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Davide Caratti @ 2021-09-01 20:53 UTC (permalink / raw)
  To: Cong Wang
  Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
	Jiri Pirko, Jakub Kicinski, Hangbin Liu, Petr Machata

On Tue, Aug 31, 2021 at 11:16:44AM -0700, Cong Wang wrote:
> On Tue, Aug 31, 2021 at 2:54 AM Davide Caratti <dcaratti@redhat.com> wrote:
> >
> > hello Cong, thanks a lot for looking at this!
> >
> > On Mon, Aug 30, 2021 at 05:43:09PM -0700, Cong Wang wrote:
> > > On Tue, Aug 24, 2021 at 3:34 PM Davide Caratti <dcaratti@redhat.com> wrote:

[...]

> > > > Then, a call to ets_qdisc_reset() will attempt to
> > > > do list_del(&alist) with 'alist' filled with zero, hence the NULL pointer
> > > > dereference.
> > >
> > > I am confused about how we end up having NULL in list head.

[...]

> > So, I can probably send a patch (for net-next, when it reopens) that removes
> > this INIT_LIST_HEAD() line; anyway, its presence is harmless IMO. WDYT?
> 
> Actually I am thinking about the opposite, that is, always initializing the
> list head. ;) Unless we always use list_add*() before list_del(), initializing
> it unconditionally is a correct fix.

uh, maybe I get the point.

we can do a INIT_LIST_HEAD(&cl[i].alist) in ets_qdisc_init() with 'i' ranging
from 0 to TCQ_ETS_MAX_BANDS - 1, then the memset() in [1] needs to be
replaced with something that clears all members of struct ets_class except
'alist'. At this point, the INIT_LIST_HEAD() line in ets_qdisc_change() can be
removed. 

I can re-run the kseltest and eventually send a patch for that (targeting
net-next, no need to rush), is that ok for you?

thanks,

-- 
davide


[1] https://elixir.bootlin.com/linux/v5.14/source/net/sched/sch_ets.c#L690


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATH net] net/sched: ets: fix crash when flipping from 'strict' to 'quantum'
  2021-09-01 20:53       ` Davide Caratti
@ 2021-09-03 23:50         ` Cong Wang
  0 siblings, 0 replies; 7+ messages in thread
From: Cong Wang @ 2021-09-03 23:50 UTC (permalink / raw)
  To: Davide Caratti
  Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
	Jiri Pirko, Jakub Kicinski, Hangbin Liu, Petr Machata

On Wed, Sep 1, 2021 at 1:54 PM Davide Caratti <dcaratti@redhat.com> wrote:
> uh, maybe I get the point.
>
> we can do a INIT_LIST_HEAD(&cl[i].alist) in ets_qdisc_init() with 'i' ranging
> from 0 to TCQ_ETS_MAX_BANDS - 1, then the memset() in [1] needs to be
> replaced with something that clears all members of struct ets_class except
> 'alist'. At this point, the INIT_LIST_HEAD() line in ets_qdisc_change() can be
> removed.
>
> I can re-run the kseltest and eventually send a patch for that (targeting
> net-next, no need to rush), is that ok for you?

Sure, whatever works for you.

Thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-03 23:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-24 22:33 [PATH net] net/sched: ets: fix crash when flipping from 'strict' to 'quantum' Davide Caratti
2021-08-25 10:20 ` patchwork-bot+netdevbpf
2021-08-31  0:43 ` Cong Wang
2021-08-31  9:54   ` Davide Caratti
2021-08-31 18:16     ` Cong Wang
2021-09-01 20:53       ` Davide Caratti
2021-09-03 23:50         ` Cong Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.