| Message ID | 20180608000636.4338-1-jacob.e.keller@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Jeff Kirsher |
| Headers | show |
| Series | fq_codel: fix NULL pointer deref in fq_codel_reset | expand |
> -----Original Message----- > From: Keller, Jacob E > Sent: Thursday, June 07, 2018 5:07 PM > To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org> > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; Keller, Jacob E > <jacob.e.keller@intel.com>; Eric Dumazet <edumazet@google.com> > Subject: [PATCH] fq_codel: fix NULL pointer deref in fq_codel_reset > This should be targeting [net] as that's where I built it, as it is a bug fix. I forgot to set the subject prefix when sending the mail. Thanks, Jake > The function qdisc_create_dftl attempts to create a default qdisc. If > this fails, it calls qdisc_destroy when cleaning up. The qdisc_destroy > function calls the ->reset op on the qdisc. > > In the case of sch_fq_codel.c, this function will panic when the qdisc > wasn't properly initialized: > > kernel: BUG: unable to handle kernel NULL pointer dereference at > 0000000000000008 > kernel: IP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] > kernel: PGD 0 P4D 0 > kernel: Oops: 0000 [#1] SMP PTI > kernel: Modules linked in: i40iw i40e(OE) xt_CHECKSUM iptable_mangle > ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat > nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack tun bridge stp llc > devlink ebtable_filter ebtables ip6table_filter ip6_tables rpcrdma ib_isert > iscsi_target_mod sunrpc ib_iser libiscsi scsi_transport_iscsi ib_srpt > target_core_mod ib_srp scsi_transport_srp ib_ipoib rdma_ucm ib_ucm > ib_uverbs ib_umad rdma_cm ib_cm iw_cm intel_rapl sb_edac > x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass > crct10dif_pclmul crc32_pclmul ghash_clmulni_intel intel_cstate iTCO_wdt > iTCO_vendor_support intel_uncore ib_core intel_rapl_perf mei_me mei joydev > i2c_i801 lpc_ich ioatdma shpchp wmi sch_fq_codel xfs libcrc32c mgag200 ixgbe > drm_kms_helper isci ttm firewire_ohci > kernel: mdio drm igb libsas crc32c_intel firewire_core ptp pps_core > scsi_transport_sas crc_itu_t dca i2c_algo_bit ipmi_si ipmi_devintf > ipmi_msghandler [last unloaded: i40e] > kernel: CPU: 10 PID: 4219 Comm: ip Tainted: G OE 4.16.13custom-fq- > codel-test+ #3 > kernel: Hardware name: Intel Corporation S2600CO/S2600CO, BIOS > SE5C600.86B.02.05.0004.051120151007 05/11/2015 > kernel: RIP: 0010:fq_codel_reset+0x58/0xd0 [sch_fq_codel] > kernel: RSP: 0018:ffffbfbf4c1fb620 EFLAGS: 00010246 > kernel: RAX: 0000000000000400 RBX: 0000000000000000 RCX: 00000000000005b9 > kernel: RDX: 0000000000000000 RSI: ffff9d03264a60c0 RDI: ffff9cfd17b31c00 > kernel: RBP: 0000000000000001 R08: 00000000000260c0 R09: ffffffffb679c3e9 > kernel: R10: fffff1dab06a0e80 R11: ffff9cfd163af800 R12: ffff9cfd17b31c00 > kernel: R13: 0000000000000001 R14: ffff9cfd153de600 R15: 0000000000000001 > kernel: FS: 00007fdec2f92800(0000) GS:ffff9d0326480000(0000) > knlGS:0000000000000000 > kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > kernel: CR2: 0000000000000008 CR3: 0000000c1956a006 CR4: 00000000000606e0 > kernel: Call Trace: > kernel: qdisc_destroy+0x56/0x140 > kernel: qdisc_create_dflt+0x8b/0xb0 > kernel: mq_init+0xc1/0xf0 > kernel: qdisc_create_dflt+0x5a/0xb0 > kernel: dev_activate+0x205/0x230 > kernel: __dev_open+0xf5/0x160 > kernel: __dev_change_flags+0x1a3/0x210 > kernel: dev_change_flags+0x21/0x60 > kernel: do_setlink+0x660/0xdf0 > kernel: ? down_trylock+0x25/0x30 > kernel: ? xfs_buf_trylock+0x1a/0xd0 [xfs] > kernel: ? rtnl_newlink+0x816/0x990 > kernel: ? _xfs_buf_find+0x327/0x580 [xfs] > kernel: ? _cond_resched+0x15/0x30 > kernel: ? kmem_cache_alloc+0x20/0x1b0 > kernel: ? rtnetlink_rcv_msg+0x200/0x2f0 > kernel: ? rtnl_calcit.isra.30+0x100/0x100 > kernel: ? netlink_rcv_skb+0x4c/0x120 > kernel: ? netlink_unicast+0x19e/0x260 > kernel: ? netlink_sendmsg+0x1ff/0x3c0 > kernel: ? sock_sendmsg+0x36/0x40 > kernel: ? ___sys_sendmsg+0x295/0x2f0 > kernel: ? ebitmap_cmp+0x6d/0x90 > kernel: ? dev_get_by_name_rcu+0x73/0x90 > kernel: ? skb_dequeue+0x52/0x60 > kernel: ? __inode_wait_for_writeback+0x7f/0xf0 > kernel: ? bit_waitqueue+0x30/0x30 > kernel: ? fsnotify_grab_connector+0x3c/0x60 > kernel: ? __sys_sendmsg+0x51/0x90 > kernel: ? do_syscall_64+0x74/0x180 > kernel: ? entry_SYSCALL_64_after_hwframe+0x3d/0xa2 > kernel: Code: 00 00 48 89 87 00 02 00 00 8b 87 a0 01 00 00 85 c0 0f 84 84 00 00 00 > 31 ed 48 63 dd 83 c5 01 48 c1 e3 06 49 03 9c 24 90 01 00 00 <48> 8b 73 08 48 8b 3b e8 > 6c 9a 4f f6 48 8d 43 10 48 c7 03 00 00 > kernel: RIP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] RSP: ffffbfbf4c1fb620 > kernel: CR2: 0000000000000008 > kernel: ---[ end trace e81a62bede66274e ]--- > > This occurs because if fq_codel_init fails, it has left the private data > in an incomplete state. For example, if tcf_block_get fails, (as in the > above panic), then q->flows and q->backlogs will be NULL. Thus they will > cause NULL pointer access when attempting to reset them in > fq_codel_reset. > > We could mitigate some of these issues by changing fq_codel_init to more > explicitly cleanup after itself when failing. For example, we could > ensure that q->flowcnt was set to 0 so that the loop over each flow in > fq_codel_reset would not trigger. However, this would not prevent a NULL > pointer dereference when attempting to memset the q->backlogs. > > Instead, just add a NULL check prior to attempting to reset these > fields. > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > Cc: Eric Dumazet <edumazet@google.com> > --- > net/sched/sch_fq_codel.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c > index 22fa13cf5d8b..1658c314ee40 100644 > --- a/net/sched/sch_fq_codel.c > +++ b/net/sched/sch_fq_codel.c > @@ -352,14 +352,17 @@ static void fq_codel_reset(struct Qdisc *sch) > > INIT_LIST_HEAD(&q->new_flows); > INIT_LIST_HEAD(&q->old_flows); > - for (i = 0; i < q->flows_cnt; i++) { > - struct fq_codel_flow *flow = q->flows + i; > + if (q->flows) { > + for (i = 0; i < q->flows_cnt; i++) { > + struct fq_codel_flow *flow = q->flows + i; > > - fq_codel_flow_purge(flow); > - INIT_LIST_HEAD(&flow->flowchain); > - codel_vars_init(&flow->cvars); > + fq_codel_flow_purge(flow); > + INIT_LIST_HEAD(&flow->flowchain); > + codel_vars_init(&flow->cvars); > + } > } > - memset(q->backlogs, 0, q->flows_cnt * sizeof(u32)); > + if (q->backlogs) > + memset(q->backlogs, 0, q->flows_cnt * sizeof(u32)); > sch->q.qlen = 0; > sch->qstats.backlog = 0; > q->memory_usage = 0; > -- > 2.18.0.rc1.134.g5f29118f3507
Not sure this is the right fix. It is very strange that ->init() (failure) -> reset() , then ->destroy() Why reset() is called before a destroy, I have no idea. Anyway you must send patches to netdev@ Thanks. On Thu, Jun 7, 2018 at 5:08 PM Keller, Jacob E <jacob.e.keller@intel.com> wrote: > > -----Original Message----- > > From: Keller, Jacob E > > Sent: Thursday, June 07, 2018 5:07 PM > > To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org> > > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; Keller, Jacob E > > <jacob.e.keller@intel.com>; Eric Dumazet <edumazet@google.com> > > Subject: [PATCH] fq_codel: fix NULL pointer deref in fq_codel_reset > > > > This should be targeting [net] as that's where I built it, as it is a bug > fix. I forgot to set the subject prefix when sending the mail. > > Thanks, > Jake > > > The function qdisc_create_dftl attempts to create a default qdisc. If > > this fails, it calls qdisc_destroy when cleaning up. The qdisc_destroy > > function calls the ->reset op on the qdisc. > > > > In the case of sch_fq_codel.c, this function will panic when the qdisc > > wasn't properly initialized: > > > > kernel: BUG: unable to handle kernel NULL pointer dereference at > > 0000000000000008 > > kernel: IP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] > > kernel: PGD 0 P4D 0 > > kernel: Oops: 0000 [#1] SMP PTI > > kernel: Modules linked in: i40iw i40e(OE) xt_CHECKSUM iptable_mangle > > ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat > > nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack tun bridge > stp llc > > devlink ebtable_filter ebtables ip6table_filter ip6_tables rpcrdma > ib_isert > > iscsi_target_mod sunrpc ib_iser libiscsi scsi_transport_iscsi ib_srpt > > target_core_mod ib_srp scsi_transport_srp ib_ipoib rdma_ucm ib_ucm > > ib_uverbs ib_umad rdma_cm ib_cm iw_cm intel_rapl sb_edac > > x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass > > crct10dif_pclmul crc32_pclmul ghash_clmulni_intel intel_cstate iTCO_wdt > > iTCO_vendor_support intel_uncore ib_core intel_rapl_perf mei_me mei > joydev > > i2c_i801 lpc_ich ioatdma shpchp wmi sch_fq_codel xfs libcrc32c mgag200 > ixgbe > > drm_kms_helper isci ttm firewire_ohci > > kernel: mdio drm igb libsas crc32c_intel firewire_core ptp pps_core > > scsi_transport_sas crc_itu_t dca i2c_algo_bit ipmi_si ipmi_devintf > > ipmi_msghandler [last unloaded: i40e] > > kernel: CPU: 10 PID: 4219 Comm: ip Tainted: G OE > 4.16.13custom-fq- > > codel-test+ #3 > > kernel: Hardware name: Intel Corporation S2600CO/S2600CO, BIOS > > SE5C600.86B.02.05.0004.051120151007 05/11/2015 > > kernel: RIP: 0010:fq_codel_reset+0x58/0xd0 [sch_fq_codel] > > kernel: RSP: 0018:ffffbfbf4c1fb620 EFLAGS: 00010246 > > kernel: RAX: 0000000000000400 RBX: 0000000000000000 RCX: > 00000000000005b9 > > kernel: RDX: 0000000000000000 RSI: ffff9d03264a60c0 RDI: > ffff9cfd17b31c00 > > kernel: RBP: 0000000000000001 R08: 00000000000260c0 R09: > ffffffffb679c3e9 > > kernel: R10: fffff1dab06a0e80 R11: ffff9cfd163af800 R12: > ffff9cfd17b31c00 > > kernel: R13: 0000000000000001 R14: ffff9cfd153de600 R15: > 0000000000000001 > > kernel: FS: 00007fdec2f92800(0000) GS:ffff9d0326480000(0000) > > knlGS:0000000000000000 > > kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > kernel: CR2: 0000000000000008 CR3: 0000000c1956a006 CR4: > 00000000000606e0 > > kernel: Call Trace: > > kernel: qdisc_destroy+0x56/0x140 > > kernel: qdisc_create_dflt+0x8b/0xb0 > > kernel: mq_init+0xc1/0xf0 > > kernel: qdisc_create_dflt+0x5a/0xb0 > > kernel: dev_activate+0x205/0x230 > > kernel: __dev_open+0xf5/0x160 > > kernel: __dev_change_flags+0x1a3/0x210 > > kernel: dev_change_flags+0x21/0x60 > > kernel: do_setlink+0x660/0xdf0 > > kernel: ? down_trylock+0x25/0x30 > > kernel: ? xfs_buf_trylock+0x1a/0xd0 [xfs] > > kernel: ? rtnl_newlink+0x816/0x990 > > kernel: ? _xfs_buf_find+0x327/0x580 [xfs] > > kernel: ? _cond_resched+0x15/0x30 > > kernel: ? kmem_cache_alloc+0x20/0x1b0 > > kernel: ? rtnetlink_rcv_msg+0x200/0x2f0 > > kernel: ? rtnl_calcit.isra.30+0x100/0x100 > > kernel: ? netlink_rcv_skb+0x4c/0x120 > > kernel: ? netlink_unicast+0x19e/0x260 > > kernel: ? netlink_sendmsg+0x1ff/0x3c0 > > kernel: ? sock_sendmsg+0x36/0x40 > > kernel: ? ___sys_sendmsg+0x295/0x2f0 > > kernel: ? ebitmap_cmp+0x6d/0x90 > > kernel: ? dev_get_by_name_rcu+0x73/0x90 > > kernel: ? skb_dequeue+0x52/0x60 > > kernel: ? __inode_wait_for_writeback+0x7f/0xf0 > > kernel: ? bit_waitqueue+0x30/0x30 > > kernel: ? fsnotify_grab_connector+0x3c/0x60 > > kernel: ? __sys_sendmsg+0x51/0x90 > > kernel: ? do_syscall_64+0x74/0x180 > > kernel: ? entry_SYSCALL_64_after_hwframe+0x3d/0xa2 > > kernel: Code: 00 00 48 89 87 00 02 00 00 8b 87 a0 01 00 00 85 c0 0f > 84 84 00 00 00 > > 31 ed 48 63 dd 83 c5 01 48 c1 e3 06 49 03 9c 24 90 01 00 00 <48> 8b 73 > 08 48 8b 3b e8 > > 6c 9a 4f f6 48 8d 43 10 48 c7 03 00 00 > > kernel: RIP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] RSP: > ffffbfbf4c1fb620 > > kernel: CR2: 0000000000000008 > > kernel: ---[ end trace e81a62bede66274e ]--- > > > > This occurs because if fq_codel_init fails, it has left the private data > > in an incomplete state. For example, if tcf_block_get fails, (as in the > > above panic), then q->flows and q->backlogs will be NULL. Thus they will > > cause NULL pointer access when attempting to reset them in > > fq_codel_reset. > > > > We could mitigate some of these issues by changing fq_codel_init to more > > explicitly cleanup after itself when failing. For example, we could > > ensure that q->flowcnt was set to 0 so that the loop over each flow in > > fq_codel_reset would not trigger. However, this would not prevent a NULL > > pointer dereference when attempting to memset the q->backlogs. > > > > Instead, just add a NULL check prior to attempting to reset these > > fields. > > > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > Cc: Eric Dumazet <edumazet@google.com> > > --- > > net/sched/sch_fq_codel.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c > > index 22fa13cf5d8b..1658c314ee40 100644 > > --- a/net/sched/sch_fq_codel.c > > +++ b/net/sched/sch_fq_codel.c > > @@ -352,14 +352,17 @@ static void fq_codel_reset(struct Qdisc *sch) > > > > INIT_LIST_HEAD(&q->new_flows); > > INIT_LIST_HEAD(&q->old_flows); > > - for (i = 0; i < q->flows_cnt; i++) { > > - struct fq_codel_flow *flow = q->flows + i; > > + if (q->flows) { > > + for (i = 0; i < q->flows_cnt; i++) { > > + struct fq_codel_flow *flow = q->flows + i; > > > > - fq_codel_flow_purge(flow); > > - INIT_LIST_HEAD(&flow->flowchain); > > - codel_vars_init(&flow->cvars); > > + fq_codel_flow_purge(flow); > > + INIT_LIST_HEAD(&flow->flowchain); > > + codel_vars_init(&flow->cvars); > > + } > > } > > - memset(q->backlogs, 0, q->flows_cnt * sizeof(u32)); > > + if (q->backlogs) > > + memset(q->backlogs, 0, q->flows_cnt * sizeof(u32)); > > sch->q.qlen = 0; > > sch->qstats.backlog = 0; > > q->memory_usage = 0; > > -- > > 2.18.0.rc1.134.g5f29118f3507 > > <div dir="ltr">Not sure this is the right fix.<div><br></div><div>It is very strange that </div><div><br></div><div>->init() (failure) -> reset() , then ->destroy()</div><div><br></div><div>Why reset() is called before a destroy, I have no idea.</div><div><br></div><div>Anyway you must send patches to netdev@</div><div><br></div><div>Thanks.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Jun 7, 2018 at 5:08 PM Keller, Jacob E <<a href="mailto:jacob.e.keller@intel.com">jacob.e.keller@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> -----Original Message-----<br> > From: Keller, Jacob E<br> > Sent: Thursday, June 07, 2018 5:07 PM<br> > To: Intel Wired LAN <<a href="mailto:intel-wired-lan@lists.osuosl.org" target="_blank">intel-wired-lan@lists.osuosl.org</a>><br> > Cc: Kirsher, Jeffrey T <<a href="mailto:jeffrey.t.kirsher@intel.com" target="_blank">jeffrey.t.kirsher@intel.com</a>>; Keller, Jacob E<br> > <<a href="mailto:jacob.e.keller@intel.com" target="_blank">jacob.e.keller@intel.com</a>>; Eric Dumazet <<a href="mailto:edumazet@google.com" target="_blank">edumazet@google.com</a>><br> > Subject: [PATCH] fq_codel: fix NULL pointer deref in fq_codel_reset<br> > <br> <br> This should be targeting [net] as that's where I built it, as it is a bug fix. I forgot to set the subject prefix when sending the mail.<br> <br> Thanks,<br> Jake<br> <br> > The function qdisc_create_dftl attempts to create a default qdisc. If<br> > this fails, it calls qdisc_destroy when cleaning up. The qdisc_destroy<br> > function calls the ->reset op on the qdisc.<br> > <br> > In the case of sch_fq_codel.c, this function will panic when the qdisc<br> > wasn't properly initialized:<br> > <br> > kernel: BUG: unable to handle kernel NULL pointer dereference at<br> > 0000000000000008<br> > kernel: IP: fq_codel_reset+0x58/0xd0 [sch_fq_codel]<br> > kernel: PGD 0 P4D 0<br> > kernel: Oops: 0000 [#1] SMP PTI<br> > kernel: Modules linked in: i40iw i40e(OE) xt_CHECKSUM iptable_mangle<br> > ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat<br> > nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack tun bridge stp llc<br> > devlink ebtable_filter ebtables ip6table_filter ip6_tables rpcrdma ib_isert<br> > iscsi_target_mod sunrpc ib_iser libiscsi scsi_transport_iscsi ib_srpt<br> > target_core_mod ib_srp scsi_transport_srp ib_ipoib rdma_ucm ib_ucm<br> > ib_uverbs ib_umad rdma_cm ib_cm iw_cm intel_rapl sb_edac<br> > x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass<br> > crct10dif_pclmul crc32_pclmul ghash_clmulni_intel intel_cstate iTCO_wdt<br> > iTCO_vendor_support intel_uncore ib_core intel_rapl_perf mei_me mei joydev<br> > i2c_i801 lpc_ich ioatdma shpchp wmi sch_fq_codel xfs libcrc32c mgag200 ixgbe<br> > drm_kms_helper isci ttm firewire_ohci<br> > kernel: mdio drm igb libsas crc32c_intel firewire_core ptp pps_core<br> > scsi_transport_sas crc_itu_t dca i2c_algo_bit ipmi_si ipmi_devintf<br> > ipmi_msghandler [last unloaded: i40e]<br> > kernel: CPU: 10 PID: 4219 Comm: ip Tainted: G OE 4.16.13custom-fq-<br> > codel-test+ #3<br> > kernel: Hardware name: Intel Corporation S2600CO/S2600CO, BIOS<br> > SE5C600.86B.02.05.0004.051120151007 05/11/2015<br> > kernel: RIP: 0010:fq_codel_reset+0x58/0xd0 [sch_fq_codel]<br> > kernel: RSP: 0018:ffffbfbf4c1fb620 EFLAGS: 00010246<br> > kernel: RAX: 0000000000000400 RBX: 0000000000000000 RCX: 00000000000005b9<br> > kernel: RDX: 0000000000000000 RSI: ffff9d03264a60c0 RDI: ffff9cfd17b31c00<br> > kernel: RBP: 0000000000000001 R08: 00000000000260c0 R09: ffffffffb679c3e9<br> > kernel: R10: fffff1dab06a0e80 R11: ffff9cfd163af800 R12: ffff9cfd17b31c00<br> > kernel: R13: 0000000000000001 R14: ffff9cfd153de600 R15: 0000000000000001<br> > kernel: FS: 00007fdec2f92800(0000) GS:ffff9d0326480000(0000)<br> > knlGS:0000000000000000<br> > kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033<br> > kernel: CR2: 0000000000000008 CR3: 0000000c1956a006 CR4: 00000000000606e0<br> > kernel: Call Trace:<br> > kernel: qdisc_destroy+0x56/0x140<br> > kernel: qdisc_create_dflt+0x8b/0xb0<br> > kernel: mq_init+0xc1/0xf0<br> > kernel: qdisc_create_dflt+0x5a/0xb0<br> > kernel: dev_activate+0x205/0x230<br> > kernel: __dev_open+0xf5/0x160<br> > kernel: __dev_change_flags+0x1a3/0x210<br> > kernel: dev_change_flags+0x21/0x60<br> > kernel: do_setlink+0x660/0xdf0<br> > kernel: ? down_trylock+0x25/0x30<br> > kernel: ? xfs_buf_trylock+0x1a/0xd0 [xfs]<br> > kernel: ? rtnl_newlink+0x816/0x990<br> > kernel: ? _xfs_buf_find+0x327/0x580 [xfs]<br> > kernel: ? _cond_resched+0x15/0x30<br> > kernel: ? kmem_cache_alloc+0x20/0x1b0<br> > kernel: ? rtnetlink_rcv_msg+0x200/0x2f0<br> > kernel: ? rtnl_calcit.isra.30+0x100/0x100<br> > kernel: ? netlink_rcv_skb+0x4c/0x120<br> > kernel: ? netlink_unicast+0x19e/0x260<br> > kernel: ? netlink_sendmsg+0x1ff/0x3c0<br> > kernel: ? sock_sendmsg+0x36/0x40<br> > kernel: ? ___sys_sendmsg+0x295/0x2f0<br> > kernel: ? ebitmap_cmp+0x6d/0x90<br> > kernel: ? dev_get_by_name_rcu+0x73/0x90<br> > kernel: ? skb_dequeue+0x52/0x60<br> > kernel: ? __inode_wait_for_writeback+0x7f/0xf0<br> > kernel: ? bit_waitqueue+0x30/0x30<br> > kernel: ? fsnotify_grab_connector+0x3c/0x60<br> > kernel: ? __sys_sendmsg+0x51/0x90<br> > kernel: ? do_syscall_64+0x74/0x180<br> > kernel: ? entry_SYSCALL_64_after_hwframe+0x3d/0xa2<br> > kernel: Code: 00 00 48 89 87 00 02 00 00 8b 87 a0 01 00 00 85 c0 0f 84 84 00 00 00<br> > 31 ed 48 63 dd 83 c5 01 48 c1 e3 06 49 03 9c 24 90 01 00 00 <48> 8b 73 08 48 8b 3b e8<br> > 6c 9a 4f f6 48 8d 43 10 48 c7 03 00 00<br> > kernel: RIP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] RSP: ffffbfbf4c1fb620<br> > kernel: CR2: 0000000000000008<br> > kernel: ---[ end trace e81a62bede66274e ]---<br> > <br> > This occurs because if fq_codel_init fails, it has left the private data<br> > in an incomplete state. For example, if tcf_block_get fails, (as in the<br> > above panic), then q->flows and q->backlogs will be NULL. Thus they will<br> > cause NULL pointer access when attempting to reset them in<br> > fq_codel_reset.<br> > <br> > We could mitigate some of these issues by changing fq_codel_init to more<br> > explicitly cleanup after itself when failing. For example, we could<br> > ensure that q->flowcnt was set to 0 so that the loop over each flow in<br> > fq_codel_reset would not trigger. However, this would not prevent a NULL<br> > pointer dereference when attempting to memset the q->backlogs.<br> > <br> > Instead, just add a NULL check prior to attempting to reset these<br> > fields.<br> > <br> > Signed-off-by: Jacob Keller <<a href="mailto:jacob.e.keller@intel.com" target="_blank">jacob.e.keller@intel.com</a>><br> > Cc: Eric Dumazet <<a href="mailto:edumazet@google.com" target="_blank">edumazet@google.com</a>><br> > ---<br> > net/sched/sch_fq_codel.c | 15 +++++++++------<br> > 1 file changed, 9 insertions(+), 6 deletions(-)<br> > <br> > diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c<br> > index 22fa13cf5d8b..1658c314ee40 100644<br> > --- a/net/sched/sch_fq_codel.c<br> > +++ b/net/sched/sch_fq_codel.c<br> > @@ -352,14 +352,17 @@ static void fq_codel_reset(struct Qdisc *sch)<br> > <br> > INIT_LIST_HEAD(&q->new_flows);<br> > INIT_LIST_HEAD(&q->old_flows);<br> > - for (i = 0; i < q->flows_cnt; i++) {<br> > - struct fq_codel_flow *flow = q->flows + i;<br> > + if (q->flows) {<br> > + for (i = 0; i < q->flows_cnt; i++) {<br> > + struct fq_codel_flow *flow = q->flows + i;<br> > <br> > - fq_codel_flow_purge(flow);<br> > - INIT_LIST_HEAD(&flow->flowchain);<br> > - codel_vars_init(&flow->cvars);<br> > + fq_codel_flow_purge(flow);<br> > + INIT_LIST_HEAD(&flow->flowchain);<br> > + codel_vars_init(&flow->cvars);<br> > + }<br> > }<br> > - memset(q->backlogs, 0, q->flows_cnt * sizeof(u32));<br> > + if (q->backlogs)<br> > + memset(q->backlogs, 0, q->flows_cnt * sizeof(u32));<br> > sch->q.qlen = 0;<br> > sch->qstats.backlog = 0;<br> > q->memory_usage = 0;<br> > --<br> > 2.18.0.rc1.134.g5f29118f3507<br> <br> </blockquote></div>
> -----Original Message----- > From: Eric Dumazet [mailto:edumazet@google.com] > Sent: Friday, June 08, 2018 8:01 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: intel-wired-lan@lists.osuosl.org; Kirsher, Jeffrey T > <jeffrey.t.kirsher@intel.com> > Subject: Re: [PATCH] fq_codel: fix NULL pointer deref in fq_codel_reset > > Not sure this is the right fix. > > It is very strange that > > ->init() (failure) -> reset() , then ->destroy() > > Why reset() is called before a destroy, I have no idea. > I don't understand either, and I'm not sure that this is the correct fix, but it seemed like the most sure way to avoid panics. > Anyway you must send patches to netdev@ > Jeff forwards them to netdev after it's passed testing here. Thanks, Jake > Thanks. > > > On Thu, Jun 7, 2018 at 5:08 PM Keller, Jacob E <jacob.e.keller@intel.com > <mailto:jacob.e.keller@intel.com> > wrote: > > > > -----Original Message----- > > From: Keller, Jacob E > > Sent: Thursday, June 07, 2018 5:07 PM > > To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org <mailto:intel- > wired-lan@lists.osuosl.org> > > > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com > <mailto:jeffrey.t.kirsher@intel.com> >; Keller, Jacob E > > <jacob.e.keller@intel.com <mailto:jacob.e.keller@intel.com> >; Eric > Dumazet <edumazet@google.com <mailto:edumazet@google.com> > > > Subject: [PATCH] fq_codel: fix NULL pointer deref in fq_codel_reset > > > > This should be targeting [net] as that's where I built it, as it is a bug fix. I > forgot to set the subject prefix when sending the mail. > > Thanks, > Jake > > > The function qdisc_create_dftl attempts to create a default qdisc. If > > this fails, it calls qdisc_destroy when cleaning up. The qdisc_destroy > > function calls the ->reset op on the qdisc. > > > > In the case of sch_fq_codel.c, this function will panic when the qdisc > > wasn't properly initialized: > > > > kernel: BUG: unable to handle kernel NULL pointer dereference at > > 0000000000000008 > > kernel: IP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] > > kernel: PGD 0 P4D 0 > > kernel: Oops: 0000 [#1] SMP PTI > > kernel: Modules linked in: i40iw i40e(OE) xt_CHECKSUM > iptable_mangle > > ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 > nf_nat > > nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack tun > bridge stp llc > > devlink ebtable_filter ebtables ip6table_filter ip6_tables rpcrdma > ib_isert > > iscsi_target_mod sunrpc ib_iser libiscsi scsi_transport_iscsi ib_srpt > > target_core_mod ib_srp scsi_transport_srp ib_ipoib rdma_ucm ib_ucm > > ib_uverbs ib_umad rdma_cm ib_cm iw_cm intel_rapl sb_edac > > x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass > > crct10dif_pclmul crc32_pclmul ghash_clmulni_intel intel_cstate > iTCO_wdt > > iTCO_vendor_support intel_uncore ib_core intel_rapl_perf mei_me > mei joydev > > i2c_i801 lpc_ich ioatdma shpchp wmi sch_fq_codel xfs libcrc32c > mgag200 ixgbe > > drm_kms_helper isci ttm firewire_ohci > > kernel: mdio drm igb libsas crc32c_intel firewire_core ptp pps_core > > scsi_transport_sas crc_itu_t dca i2c_algo_bit ipmi_si ipmi_devintf > > ipmi_msghandler [last unloaded: i40e] > > kernel: CPU: 10 PID: 4219 Comm: ip Tainted: G OE > 4.16.13custom-fq- > > codel-test+ #3 > > kernel: Hardware name: Intel Corporation S2600CO/S2600CO, BIOS > > SE5C600.86B.02.05.0004.051120151007 05/11/2015 > > kernel: RIP: 0010:fq_codel_reset+0x58/0xd0 [sch_fq_codel] > > kernel: RSP: 0018:ffffbfbf4c1fb620 EFLAGS: 00010246 > > kernel: RAX: 0000000000000400 RBX: 0000000000000000 RCX: > 00000000000005b9 > > kernel: RDX: 0000000000000000 RSI: ffff9d03264a60c0 RDI: > ffff9cfd17b31c00 > > kernel: RBP: 0000000000000001 R08: 00000000000260c0 R09: > ffffffffb679c3e9 > > kernel: R10: fffff1dab06a0e80 R11: ffff9cfd163af800 R12: > ffff9cfd17b31c00 > > kernel: R13: 0000000000000001 R14: ffff9cfd153de600 R15: > 0000000000000001 > > kernel: FS: 00007fdec2f92800(0000) GS:ffff9d0326480000(0000) > > knlGS:0000000000000000 > > kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > kernel: CR2: 0000000000000008 CR3: 0000000c1956a006 CR4: > 00000000000606e0 > > kernel: Call Trace: > > kernel: qdisc_destroy+0x56/0x140 > > kernel: qdisc_create_dflt+0x8b/0xb0 > > kernel: mq_init+0xc1/0xf0 > > kernel: qdisc_create_dflt+0x5a/0xb0 > > kernel: dev_activate+0x205/0x230 > > kernel: __dev_open+0xf5/0x160 > > kernel: __dev_change_flags+0x1a3/0x210 > > kernel: dev_change_flags+0x21/0x60 > > kernel: do_setlink+0x660/0xdf0 > > kernel: ? down_trylock+0x25/0x30 > > kernel: ? xfs_buf_trylock+0x1a/0xd0 [xfs] > > kernel: ? rtnl_newlink+0x816/0x990 > > kernel: ? _xfs_buf_find+0x327/0x580 [xfs] > > kernel: ? _cond_resched+0x15/0x30 > > kernel: ? kmem_cache_alloc+0x20/0x1b0 > > kernel: ? rtnetlink_rcv_msg+0x200/0x2f0 > > kernel: ? rtnl_calcit.isra.30+0x100/0x100 > > kernel: ? netlink_rcv_skb+0x4c/0x120 > > kernel: ? netlink_unicast+0x19e/0x260 > > kernel: ? netlink_sendmsg+0x1ff/0x3c0 > > kernel: ? sock_sendmsg+0x36/0x40 > > kernel: ? ___sys_sendmsg+0x295/0x2f0 > > kernel: ? ebitmap_cmp+0x6d/0x90 > > kernel: ? dev_get_by_name_rcu+0x73/0x90 > > kernel: ? skb_dequeue+0x52/0x60 > > kernel: ? __inode_wait_for_writeback+0x7f/0xf0 > > kernel: ? bit_waitqueue+0x30/0x30 > > kernel: ? fsnotify_grab_connector+0x3c/0x60 > > kernel: ? __sys_sendmsg+0x51/0x90 > > kernel: ? do_syscall_64+0x74/0x180 > > kernel: ? entry_SYSCALL_64_after_hwframe+0x3d/0xa2 > > kernel: Code: 00 00 48 89 87 00 02 00 00 8b 87 a0 01 00 00 85 c0 0f 84 84 > 00 00 00 > > 31 ed 48 63 dd 83 c5 01 48 c1 e3 06 49 03 9c 24 90 01 00 00 <48> 8b 73 08 > 48 8b 3b e8 > > 6c 9a 4f f6 48 8d 43 10 48 c7 03 00 00 > > kernel: RIP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] RSP: > ffffbfbf4c1fb620 > > kernel: CR2: 0000000000000008 > > kernel: ---[ end trace e81a62bede66274e ]--- > > > > This occurs because if fq_codel_init fails, it has left the private data > > in an incomplete state. For example, if tcf_block_get fails, (as in the > > above panic), then q->flows and q->backlogs will be NULL. Thus they > will > > cause NULL pointer access when attempting to reset them in > > fq_codel_reset. > > > > We could mitigate some of these issues by changing fq_codel_init to > more > > explicitly cleanup after itself when failing. For example, we could > > ensure that q->flowcnt was set to 0 so that the loop over each flow in > > fq_codel_reset would not trigger. However, this would not prevent a > NULL > > pointer dereference when attempting to memset the q->backlogs. > > > > Instead, just add a NULL check prior to attempting to reset these > > fields. > > > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com > <mailto:jacob.e.keller@intel.com> > > > Cc: Eric Dumazet <edumazet@google.com > <mailto:edumazet@google.com> > > > --- > > net/sched/sch_fq_codel.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c > > index 22fa13cf5d8b..1658c314ee40 100644 > > --- a/net/sched/sch_fq_codel.c > > +++ b/net/sched/sch_fq_codel.c > > @@ -352,14 +352,17 @@ static void fq_codel_reset(struct Qdisc *sch) > > > > INIT_LIST_HEAD(&q->new_flows); > > INIT_LIST_HEAD(&q->old_flows); > > - for (i = 0; i < q->flows_cnt; i++) { > > - struct fq_codel_flow *flow = q->flows + i; > > + if (q->flows) { > > + for (i = 0; i < q->flows_cnt; i++) { > > + struct fq_codel_flow *flow = q->flows + i; > > > > - fq_codel_flow_purge(flow); > > - INIT_LIST_HEAD(&flow->flowchain); > > - codel_vars_init(&flow->cvars); > > + fq_codel_flow_purge(flow); > > + INIT_LIST_HEAD(&flow->flowchain); > > + codel_vars_init(&flow->cvars); > > + } > > } > > - memset(q->backlogs, 0, q->flows_cnt * sizeof(u32)); > > + if (q->backlogs) > > + memset(q->backlogs, 0, q->flows_cnt * sizeof(u32)); > > sch->q.qlen = 0; > > sch->qstats.backlog = 0; > > q->memory_usage = 0; > > -- > > 2.18.0.rc1.134.g5f29118f3507 > >
> -----Original Message----- > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On > Behalf Of Jacob Keller > Sent: Thursday, June 7, 2018 5:07 PM > To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org> > Cc: Eric Dumazet <edumazet@google.com> > Subject: [Intel-wired-lan] [PATCH] fq_codel: fix NULL pointer deref in > fq_codel_reset > > The function qdisc_create_dftl attempts to create a default qdisc. If this fails, > it calls qdisc_destroy when cleaning up. The qdisc_destroy function calls the - > >reset op on the qdisc. > > In the case of sch_fq_codel.c, this function will panic when the qdisc wasn't > properly initialized: > > kernel: BUG: unable to handle kernel NULL pointer dereference at > 0000000000000008 > kernel: IP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] > kernel: PGD 0 P4D 0 > kernel: Oops: 0000 [#1] SMP PTI > kernel: Modules linked in: i40iw i40e(OE) xt_CHECKSUM iptable_mangle > ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat > nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack tun bridge stp > llc devlink ebtable_filter ebtables ip6table_filter ip6_tables rpcrdma ib_isert > iscsi_target_mod sunrpc ib_iser libiscsi scsi_transport_iscsi ib_srpt > target_core_mod ib_srp scsi_transport_srp ib_ipoib rdma_ucm ib_ucm > ib_uverbs ib_umad rdma_cm ib_cm iw_cm intel_rapl sb_edac > x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass > crct10dif_pclmul crc32_pclmul ghash_clmulni_intel intel_cstate iTCO_wdt > iTCO_vendor_support intel_uncore ib_core intel_rapl_perf mei_me mei > joydev i2c_i801 lpc_ich ioatdma shpchp wmi sch_fq_codel xfs libcrc32c > mgag200 ixgbe drm_kms_helper isci ttm firewire_ohci > kernel: mdio drm igb libsas crc32c_intel firewire_core ptp pps_core > scsi_transport_sas crc_itu_t dca i2c_algo_bit ipmi_si ipmi_devintf > ipmi_msghandler [last unloaded: i40e] > kernel: CPU: 10 PID: 4219 Comm: ip Tainted: G OE 4.16.13custom-fq- > codel-test+ #3 > kernel: Hardware name: Intel Corporation S2600CO/S2600CO, BIOS > SE5C600.86B.02.05.0004.051120151007 05/11/2015 > kernel: RIP: 0010:fq_codel_reset+0x58/0xd0 [sch_fq_codel] > kernel: RSP: 0018:ffffbfbf4c1fb620 EFLAGS: 00010246 > kernel: RAX: 0000000000000400 RBX: 0000000000000000 RCX: > 00000000000005b9 > kernel: RDX: 0000000000000000 RSI: ffff9d03264a60c0 RDI: ffff9cfd17b31c00 > kernel: RBP: 0000000000000001 R08: 00000000000260c0 R09: ffffffffb679c3e9 > kernel: R10: fffff1dab06a0e80 R11: ffff9cfd163af800 R12: ffff9cfd17b31c00 > kernel: R13: 0000000000000001 R14: ffff9cfd153de600 R15: > 0000000000000001 > kernel: FS: 00007fdec2f92800(0000) GS:ffff9d0326480000(0000) > knlGS:0000000000000000 > kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > kernel: CR2: 0000000000000008 CR3: 0000000c1956a006 CR4: > 00000000000606e0 > kernel: Call Trace: > kernel: qdisc_destroy+0x56/0x140 > kernel: qdisc_create_dflt+0x8b/0xb0 > kernel: mq_init+0xc1/0xf0 > kernel: qdisc_create_dflt+0x5a/0xb0 > kernel: dev_activate+0x205/0x230 > kernel: __dev_open+0xf5/0x160 > kernel: __dev_change_flags+0x1a3/0x210 > kernel: dev_change_flags+0x21/0x60 > kernel: do_setlink+0x660/0xdf0 > kernel: ? down_trylock+0x25/0x30 > kernel: ? xfs_buf_trylock+0x1a/0xd0 [xfs] > kernel: ? rtnl_newlink+0x816/0x990 > kernel: ? _xfs_buf_find+0x327/0x580 [xfs] > kernel: ? _cond_resched+0x15/0x30 > kernel: ? kmem_cache_alloc+0x20/0x1b0 > kernel: ? rtnetlink_rcv_msg+0x200/0x2f0 > kernel: ? rtnl_calcit.isra.30+0x100/0x100 > kernel: ? netlink_rcv_skb+0x4c/0x120 > kernel: ? netlink_unicast+0x19e/0x260 > kernel: ? netlink_sendmsg+0x1ff/0x3c0 > kernel: ? sock_sendmsg+0x36/0x40 > kernel: ? ___sys_sendmsg+0x295/0x2f0 > kernel: ? ebitmap_cmp+0x6d/0x90 > kernel: ? dev_get_by_name_rcu+0x73/0x90 > kernel: ? skb_dequeue+0x52/0x60 > kernel: ? __inode_wait_for_writeback+0x7f/0xf0 > kernel: ? bit_waitqueue+0x30/0x30 > kernel: ? fsnotify_grab_connector+0x3c/0x60 > kernel: ? __sys_sendmsg+0x51/0x90 > kernel: ? do_syscall_64+0x74/0x180 > kernel: ? entry_SYSCALL_64_after_hwframe+0x3d/0xa2 > kernel: Code: 00 00 48 89 87 00 02 00 00 8b 87 a0 01 00 00 85 c0 0f 84 84 00 00 > 00 31 ed 48 63 dd 83 c5 01 48 c1 e3 06 49 03 9c 24 90 01 00 00 <48> 8b 73 08 48 > 8b 3b e8 6c 9a 4f f6 48 8d 43 10 48 c7 03 00 00 > kernel: RIP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] RSP: > ffffbfbf4c1fb620 > kernel: CR2: 0000000000000008 > kernel: ---[ end trace e81a62bede66274e ]--- > > This occurs because if fq_codel_init fails, it has left the private data in an > incomplete state. For example, if tcf_block_get fails, (as in the above panic), > then q->flows and q->backlogs will be NULL. Thus they will cause NULL > pointer access when attempting to reset them in fq_codel_reset. > > We could mitigate some of these issues by changing fq_codel_init to more > explicitly cleanup after itself when failing. For example, we could ensure that > q->flowcnt was set to 0 so that the loop over each flow in fq_codel_reset > would not trigger. However, this would not prevent a NULL pointer > dereference when attempting to memset the q->backlogs. > > Instead, just add a NULL check prior to attempting to reset these fields. > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > Cc: Eric Dumazet <edumazet@google.com> > --- > net/sched/sch_fq_codel.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> -----Original Message----- > From: Eric Dumazet [mailto:edumazet@google.com] > Sent: Friday, June 08, 2018 8:01 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: intel-wired-lan@lists.osuosl.org; Kirsher, Jeffrey T > <jeffrey.t.kirsher@intel.com> > Subject: Re: [PATCH] fq_codel: fix NULL pointer deref in fq_codel_reset > > Not sure this is the right fix. > > It is very strange that > > ->init() (failure) -> reset() , then ->destroy() > > Why reset() is called before a destroy, I have no idea. > > Anyway you must send patches to netdev@ > > Thanks. If you have a better suggestion, I'm all ears. I've heard that we should set qflows_cnt to 0 instead of checking the null pointer directly. I'm fine with that solution too. I'd be fine removing the reset() call also, if you think that's better. I don't know if that would adversely impact the other schedulers though. Thanks, Jake
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c index 22fa13cf5d8b..1658c314ee40 100644 --- a/net/sched/sch_fq_codel.c +++ b/net/sched/sch_fq_codel.c @@ -352,14 +352,17 @@ static void fq_codel_reset(struct Qdisc *sch) INIT_LIST_HEAD(&q->new_flows); INIT_LIST_HEAD(&q->old_flows); - for (i = 0; i < q->flows_cnt; i++) { - struct fq_codel_flow *flow = q->flows + i; + if (q->flows) { + for (i = 0; i < q->flows_cnt; i++) { + struct fq_codel_flow *flow = q->flows + i; - fq_codel_flow_purge(flow); - INIT_LIST_HEAD(&flow->flowchain); - codel_vars_init(&flow->cvars); + fq_codel_flow_purge(flow); + INIT_LIST_HEAD(&flow->flowchain); + codel_vars_init(&flow->cvars); + } } - memset(q->backlogs, 0, q->flows_cnt * sizeof(u32)); + if (q->backlogs) + memset(q->backlogs, 0, q->flows_cnt * sizeof(u32)); sch->q.qlen = 0; sch->qstats.backlog = 0; q->memory_usage = 0;
The function qdisc_create_dftl attempts to create a default qdisc. If this fails, it calls qdisc_destroy when cleaning up. The qdisc_destroy function calls the ->reset op on the qdisc. In the case of sch_fq_codel.c, this function will panic when the qdisc wasn't properly initialized: kernel: BUG: unable to handle kernel NULL pointer dereference at 0000000000000008 kernel: IP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] kernel: PGD 0 P4D 0 kernel: Oops: 0000 [#1] SMP PTI kernel: Modules linked in: i40iw i40e(OE) xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack tun bridge stp llc devlink ebtable_filter ebtables ip6table_filter ip6_tables rpcrdma ib_isert iscsi_target_mod sunrpc ib_iser libiscsi scsi_transport_iscsi ib_srpt target_core_mod ib_srp scsi_transport_srp ib_ipoib rdma_ucm ib_ucm ib_uverbs ib_umad rdma_cm ib_cm iw_cm intel_rapl sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel intel_cstate iTCO_wdt iTCO_vendor_support intel_uncore ib_core intel_rapl_perf mei_me mei joydev i2c_i801 lpc_ich ioatdma shpchp wmi sch_fq_codel xfs libcrc32c mgag200 ixgbe drm_kms_helper isci ttm firewire_ohci kernel: mdio drm igb libsas crc32c_intel firewire_core ptp pps_core scsi_transport_sas crc_itu_t dca i2c_algo_bit ipmi_si ipmi_devintf ipmi_msghandler [last unloaded: i40e] kernel: CPU: 10 PID: 4219 Comm: ip Tainted: G OE 4.16.13custom-fq-codel-test+ #3 kernel: Hardware name: Intel Corporation S2600CO/S2600CO, BIOS SE5C600.86B.02.05.0004.051120151007 05/11/2015 kernel: RIP: 0010:fq_codel_reset+0x58/0xd0 [sch_fq_codel] kernel: RSP: 0018:ffffbfbf4c1fb620 EFLAGS: 00010246 kernel: RAX: 0000000000000400 RBX: 0000000000000000 RCX: 00000000000005b9 kernel: RDX: 0000000000000000 RSI: ffff9d03264a60c0 RDI: ffff9cfd17b31c00 kernel: RBP: 0000000000000001 R08: 00000000000260c0 R09: ffffffffb679c3e9 kernel: R10: fffff1dab06a0e80 R11: ffff9cfd163af800 R12: ffff9cfd17b31c00 kernel: R13: 0000000000000001 R14: ffff9cfd153de600 R15: 0000000000000001 kernel: FS: 00007fdec2f92800(0000) GS:ffff9d0326480000(0000) knlGS:0000000000000000 kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 kernel: CR2: 0000000000000008 CR3: 0000000c1956a006 CR4: 00000000000606e0 kernel: Call Trace: kernel: qdisc_destroy+0x56/0x140 kernel: qdisc_create_dflt+0x8b/0xb0 kernel: mq_init+0xc1/0xf0 kernel: qdisc_create_dflt+0x5a/0xb0 kernel: dev_activate+0x205/0x230 kernel: __dev_open+0xf5/0x160 kernel: __dev_change_flags+0x1a3/0x210 kernel: dev_change_flags+0x21/0x60 kernel: do_setlink+0x660/0xdf0 kernel: ? down_trylock+0x25/0x30 kernel: ? xfs_buf_trylock+0x1a/0xd0 [xfs] kernel: ? rtnl_newlink+0x816/0x990 kernel: ? _xfs_buf_find+0x327/0x580 [xfs] kernel: ? _cond_resched+0x15/0x30 kernel: ? kmem_cache_alloc+0x20/0x1b0 kernel: ? rtnetlink_rcv_msg+0x200/0x2f0 kernel: ? rtnl_calcit.isra.30+0x100/0x100 kernel: ? netlink_rcv_skb+0x4c/0x120 kernel: ? netlink_unicast+0x19e/0x260 kernel: ? netlink_sendmsg+0x1ff/0x3c0 kernel: ? sock_sendmsg+0x36/0x40 kernel: ? ___sys_sendmsg+0x295/0x2f0 kernel: ? ebitmap_cmp+0x6d/0x90 kernel: ? dev_get_by_name_rcu+0x73/0x90 kernel: ? skb_dequeue+0x52/0x60 kernel: ? __inode_wait_for_writeback+0x7f/0xf0 kernel: ? bit_waitqueue+0x30/0x30 kernel: ? fsnotify_grab_connector+0x3c/0x60 kernel: ? __sys_sendmsg+0x51/0x90 kernel: ? do_syscall_64+0x74/0x180 kernel: ? entry_SYSCALL_64_after_hwframe+0x3d/0xa2 kernel: Code: 00 00 48 89 87 00 02 00 00 8b 87 a0 01 00 00 85 c0 0f 84 84 00 00 00 31 ed 48 63 dd 83 c5 01 48 c1 e3 06 49 03 9c 24 90 01 00 00 <48> 8b 73 08 48 8b 3b e8 6c 9a 4f f6 48 8d 43 10 48 c7 03 00 00 kernel: RIP: fq_codel_reset+0x58/0xd0 [sch_fq_codel] RSP: ffffbfbf4c1fb620 kernel: CR2: 0000000000000008 kernel: ---[ end trace e81a62bede66274e ]--- This occurs because if fq_codel_init fails, it has left the private data in an incomplete state. For example, if tcf_block_get fails, (as in the above panic), then q->flows and q->backlogs will be NULL. Thus they will cause NULL pointer access when attempting to reset them in fq_codel_reset. We could mitigate some of these issues by changing fq_codel_init to more explicitly cleanup after itself when failing. For example, we could ensure that q->flowcnt was set to 0 so that the loop over each flow in fq_codel_reset would not trigger. However, this would not prevent a NULL pointer dereference when attempting to memset the q->backlogs. Instead, just add a NULL check prior to attempting to reset these fields. Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Cc: Eric Dumazet <edumazet@google.com> --- net/sched/sch_fq_codel.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)