Message ID | 20190429173805.4455-1-mcroce@redhat.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [net] cls_matchall: avoid panic when receiving a packet before filter set | expand |
On Mon, Apr 29, 2019 at 10:38 AM Matteo Croce <mcroce@redhat.com> wrote: > > When a matchall classifier is added, there is a small time interval in > which tp->root is NULL. If we receive a packet in this small time slice > a NULL pointer dereference will happen, leading to a kernel panic: Hmm, why not just check tp->root against NULL in mall_classify()? Also, which is the offending commit here? Please add a Fixes: tag. Thanks.
On Tue, Apr 30, 2019 at 11:25 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Mon, Apr 29, 2019 at 10:38 AM Matteo Croce <mcroce@redhat.com> wrote: > > > > When a matchall classifier is added, there is a small time interval in > > which tp->root is NULL. If we receive a packet in this small time slice > > a NULL pointer dereference will happen, leading to a kernel panic: > > Hmm, why not just check tp->root against NULL in mall_classify()? > > Also, which is the offending commit here? Please add a Fixes: tag. > > Thanks. Hi, I just want to avoid an extra check which would be made for every packet. Probably the benefit over a check is negligible, but it's still a per-packet thing. If you prefer a simple check, I can make a v2 that way. For the fixes tag, I didn't put it as I'm not really sure about the offending commit. I guess it's the following, what do you think? commit ed76f5edccc98fa66f2337f0b3b255d6e1a568b7 Author: Vlad Buslov <vladbu@mellanox.com> Date: Mon Feb 11 10:55:38 2019 +0200 net: sched: protect filter_chain list with filter_chain_lock mutex
On Wed, May 1, 2019 at 2:27 AM Matteo Croce <mcroce@redhat.com> wrote: > > On Tue, Apr 30, 2019 at 11:25 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > On Mon, Apr 29, 2019 at 10:38 AM Matteo Croce <mcroce@redhat.com> wrote: > > > > > > When a matchall classifier is added, there is a small time interval in > > > which tp->root is NULL. If we receive a packet in this small time slice > > > a NULL pointer dereference will happen, leading to a kernel panic: > > > > Hmm, why not just check tp->root against NULL in mall_classify()? > > > > Also, which is the offending commit here? Please add a Fixes: tag. > > > > Thanks. > > Hi, > > I just want to avoid an extra check which would be made for every packet. > Probably the benefit over a check is negligible, but it's still a > per-packet thing. > If you prefer a simple check, I can make a v2 that way. Yeah, I think that is better, you can add an unlikely() for performance concern, as NULL is a rare case. > > For the fixes tag, I didn't put it as I'm not really sure about the > offending commit. I guess it's the following, what do you think? > > commit ed76f5edccc98fa66f2337f0b3b255d6e1a568b7 > Author: Vlad Buslov <vladbu@mellanox.com> > Date: Mon Feb 11 10:55:38 2019 +0200 > > net: sched: protect filter_chain list with filter_chain_lock mutex I think you are right, this is the commit introduced the code that inserts the tp before fully initializing it. Please Cc Vlad for your v2, in case we blame a wrong commit here. BTW, it looks like cls_cgroup needs a same fix. Please audit other tc filters as well. Thanks!
On Thu 02 May 2019 at 03:48, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Wed, May 1, 2019 at 2:27 AM Matteo Croce <mcroce@redhat.com> wrote: >> >> On Tue, Apr 30, 2019 at 11:25 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: >> > >> > On Mon, Apr 29, 2019 at 10:38 AM Matteo Croce <mcroce@redhat.com> wrote: >> > > >> > > When a matchall classifier is added, there is a small time interval in >> > > which tp->root is NULL. If we receive a packet in this small time slice >> > > a NULL pointer dereference will happen, leading to a kernel panic: >> > >> > Hmm, why not just check tp->root against NULL in mall_classify()? >> > >> > Also, which is the offending commit here? Please add a Fixes: tag. >> > >> > Thanks. >> >> Hi, >> >> I just want to avoid an extra check which would be made for every packet. >> Probably the benefit over a check is negligible, but it's still a >> per-packet thing. >> If you prefer a simple check, I can make a v2 that way. > > Yeah, I think that is better, you can add an unlikely() for performance > concern, as NULL is a rare case. > > >> >> For the fixes tag, I didn't put it as I'm not really sure about the >> offending commit. I guess it's the following, what do you think? >> >> commit ed76f5edccc98fa66f2337f0b3b255d6e1a568b7 >> Author: Vlad Buslov <vladbu@mellanox.com> >> Date: Mon Feb 11 10:55:38 2019 +0200 >> >> net: sched: protect filter_chain list with filter_chain_lock mutex > > I think you are right, this is the commit introduced the code > that inserts the tp before fully initializing it. Please Cc Vlad > for your v2, in case we blame a wrong commit here. > > > BTW, it looks like cls_cgroup needs a same fix. Please audit > other tc filters as well. > > Thanks! Sorry for late response. This is indeed the offending commit that should be referenced by fixes tag. Thanks for fixing this, Matteo!
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c index a13bc351a414..8aaa9d80c2ca 100644 --- a/net/sched/cls_matchall.c +++ b/net/sched/cls_matchall.c @@ -27,6 +27,10 @@ struct cls_mall_head { struct rcu_work rwork; }; +static const struct cls_mall_head match_none = { + .flags = TCA_CLS_FLAGS_SKIP_SW +}; + static int mall_classify(struct sk_buff *skb, const struct tcf_proto *tp, struct tcf_result *res) { @@ -42,6 +46,8 @@ static int mall_classify(struct sk_buff *skb, const struct tcf_proto *tp, static int mall_init(struct tcf_proto *tp) { + rcu_assign_pointer(tp->root, &match_none); + return 0; } @@ -114,7 +120,7 @@ static void mall_destroy(struct tcf_proto *tp, bool rtnl_held, { struct cls_mall_head *head = rtnl_dereference(tp->root); - if (!head) + if (head == &match_none) return; tcf_unbind_filter(tp, &head->res); @@ -132,7 +138,7 @@ static void *mall_get(struct tcf_proto *tp, u32 handle) { struct cls_mall_head *head = rtnl_dereference(tp->root); - if (head && head->handle == handle) + if (head != &match_none && head->handle == handle) return head; return NULL; @@ -178,7 +184,7 @@ static int mall_change(struct net *net, struct sk_buff *in_skb, if (!tca[TCA_OPTIONS]) return -EINVAL; - if (head) + if (head != &match_none) return -EEXIST; err = nla_parse_nested(tb, TCA_MATCHALL_MAX, tca[TCA_OPTIONS], @@ -253,7 +259,7 @@ static void mall_walk(struct tcf_proto *tp, struct tcf_walker *arg, if (arg->count < arg->skip) goto skip; - if (!head) + if (head == &match_none) return; if (arg->fn(tp, head, arg) < 0) arg->stop = 1; @@ -298,7 +304,7 @@ static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh, struct nlattr *nest; int cpu; - if (!head) + if (!head || head == &match_none) return skb->len; t->tcm_handle = head->handle;
When a matchall classifier is added, there is a small time interval in which tp->root is NULL. If we receive a packet in this small time slice a NULL pointer dereference will happen, leading to a kernel panic: # tc qdisc replace dev eth0 ingress # tc filter add dev eth0 parent ffff: matchall action gact drop Unable to handle kernel NULL pointer dereference at virtual address 0000000000000034 Mem abort info: ESR = 0x96000005 Exception class = DABT (current EL), IL = 32 bits SET = 0, FnV = 0 EA = 0, S1PTW = 0 Data abort info: ISV = 0, ISS = 0x00000005 CM = 0, WnR = 0 user pgtable: 4k pages, 39-bit VAs, pgdp = 00000000a623d530 [0000000000000034] pgd=0000000000000000, pud=0000000000000000 Internal error: Oops: 96000005 [#1] SMP Modules linked in: cls_matchall sch_ingress nls_iso8859_1 nls_cp437 vfat fat m25p80 spi_nor mtd xhci_plat_hcd xhci_hcd phy_generic sfp mdio_i2c usbcore i2c_mv64xxx marvell10g mvpp2 usb_common spi_orion mvmdio i2c_core sbsa_gwdt phylink ip_tables x_tables autofs4 Process ksoftirqd/0 (pid: 9, stack limit = 0x0000000009de7d62) CPU: 0 PID: 9 Comm: ksoftirqd/0 Not tainted 5.1.0-rc6 #21 Hardware name: Marvell 8040 MACCHIATOBin Double-shot (DT) pstate: 40000005 (nZcv daif -PAN -UAO) pc : mall_classify+0x28/0x78 [cls_matchall] lr : tcf_classify+0x78/0x138 sp : ffffff80109db9d0 x29: ffffff80109db9d0 x28: ffffffc426058800 x27: 0000000000000000 x26: ffffffc425b0dd00 x25: 0000000020000000 x24: 0000000000000000 x23: ffffff80109dbac0 x22: 0000000000000001 x21: ffffffc428ab5100 x20: ffffffc425b0dd00 x19: ffffff80109dbac0 x18: 0000000000000000 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: 0000000000000000 x13: ffffffbf108ad288 x12: dead000000000200 x11: 00000000f0000000 x10: 0000000000000001 x9 : ffffffbf1089a220 x8 : 0000000000000001 x7 : ffffffbebffaa950 x6 : 0000000000000000 x5 : 000000442d6ba000 x4 : 0000000000000000 x3 : ffffff8008735ad8 x2 : ffffff80109dbac0 x1 : ffffffc425b0dd00 x0 : ffffff8010592078 Call trace: mall_classify+0x28/0x78 [cls_matchall] tcf_classify+0x78/0x138 __netif_receive_skb_core+0x29c/0xa20 __netif_receive_skb_one_core+0x34/0x60 __netif_receive_skb+0x28/0x78 netif_receive_skb_internal+0x2c/0xc0 napi_gro_receive+0x1a0/0x1d8 mvpp2_poll+0x928/0xb18 [mvpp2] net_rx_action+0x108/0x378 __do_softirq+0x128/0x320 run_ksoftirqd+0x44/0x60 smpboot_thread_fn+0x168/0x1b0 kthread+0x12c/0x130 ret_from_fork+0x10/0x1c Code: aa0203f3 aa1e03e0 d503201f f9400684 (b9403480) ---[ end trace fc71e2ef7b8ab5a5 ]--- Kernel panic - not syncing: Fatal exception in interrupt SMP: stopping secondary CPUs Kernel Offset: disabled CPU features: 0x002,00002000 Memory Limit: none Rebooting in 1 seconds.. Fix this by creating a dummy action in the init which is skipped by mall_classify, and remove it upon the first action set in mall_change. Doing this we avoid adding an extra check in the classify. Signed-off-by: Matteo Croce <mcroce@redhat.com> --- net/sched/cls_matchall.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)