* [PATCH net 1/1] net: sched: fix cleanup NULL pointer exception in act_mirr
@ 2019-03-20 18:12 John Hurley
2019-03-21 18:05 ` Cong Wang
2019-03-21 20:37 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: John Hurley @ 2019-03-20 18:12 UTC (permalink / raw)
To: jiri, davem, xiyou.wangcong; +Cc: netdev, vladbu, oss-drivers, John Hurley
A new mirred action is created by the tcf_mirred_init function. This
contains a list head struct which is inserted into a global list on
successful creation of a new action. However, after a creation, it is
still possible to error out if the egress device does not exist. This
calls the act_mirr cleanup function via __tcf_idr_release and
__tcf_action_put. This cleanup function tries to delete the list entry
which is as yet uninitialised, leading to a NULL pointer exception.
Fix this by taking a reference to the egress device, if one exists in the
params, prior to the creation of the action. This means we can error out
correctly on failure to get the device and removes the incorrect error
path from further down the function.
Bug report:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
PGD 8000000840c73067 P4D 8000000840c73067 PUD 858dcc067 PMD 0
Oops: 0002 [#1] SMP PTI
CPU: 32 PID: 5636 Comm: handler194 Tainted: G OE 5.0.0+ #186
Hardware name: Dell Inc. PowerEdge R730/0599V5, BIOS 1.3.6 06/03/2015
RIP: 0010:tcf_mirred_release+0x42/0xa7 [act_mirred]
Code: f0 90 39 c0 e8 52 04 57 c8 48 c7 c7 b8 80 39 c0 e8 94 fa d4 c7 48 8b 93 d0 00 00 00 48 8b 83 d8 00 00 00 48 c7 c7 f0 90 39 c0 <48> 89 42 08 48 89 10 48 b8 00 01 00 00 00 00 ad de 48 89 83 d0 00
RSP: 0018:ffffac4aa059f688 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff9dcd1b214d00 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff9dcd1fa165f8 RDI: ffffffffc03990f0
RBP: ffff9dccf9c7af80 R08: 0000000000000a3b R09: 0000000000000000
R10: ffff9dccfa11f420 R11: 0000000000000000 R12: 0000000000000001
R13: ffff9dcd16b433c0 R14: ffff9dcd1b214d80 R15: 0000000000000000
FS: 00007f441bfff700(0000) GS:ffff9dcd1fa00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000008 CR3: 0000000839e64004 CR4: 00000000001606e0
Call Trace:
tcf_action_cleanup+0x59/0xca
__tcf_action_put+0x54/0x6b
__tcf_idr_release.cold.33+0x9/0x12
tcf_mirred_init.cold.20+0x22e/0x3b0 [act_mirred]
tcf_action_init_1+0x3d0/0x4c0
tcf_action_init+0x9c/0x130
tcf_exts_validate+0xab/0xc0
fl_change+0x1ca/0x982 [cls_flower]
tc_new_tfilter+0x647/0x8d0
? load_balance+0x14b/0x9e0
rtnetlink_rcv_msg+0xe3/0x370
? __switch_to_asm+0x40/0x70
? __switch_to_asm+0x34/0x70
? _cond_resched+0x15/0x30
? __kmalloc_node_track_caller+0x1d4/0x2b0
? rtnl_calcit.isra.31+0xf0/0xf0
netlink_rcv_skb+0x49/0x110
netlink_unicast+0x16f/0x210
netlink_sendmsg+0x1df/0x390
sock_sendmsg+0x36/0x40
___sys_sendmsg+0x27b/0x2c0
? futex_wake+0x80/0x140
? do_futex+0x2b9/0xac0
? ep_scan_ready_list.constprop.22+0x1f2/0x210
? ep_poll+0x7a/0x430
__sys_sendmsg+0x47/0x80
do_syscall_64+0x55/0x100
entry_SYSCALL_64_after_hwframe+0x44/0xa9
Fixes: 4e232818bd32 ("net: sched: act_mirred: remove dependency on rtnl lock")
Signed-off-by: John Hurley <john.hurley@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
net/sched/act_mirred.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 6692fd0..b26b9d8 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -140,21 +140,39 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
return -EINVAL;
}
- if (!exists) {
- if (!parm->ifindex) {
- tcf_idr_cleanup(tn, parm->index);
+ if (parm->ifindex) {
+ dev = dev_get_by_index(net, parm->ifindex);
+ if (!dev) {
+ if (exists)
+ tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, parm->index);
NL_SET_ERR_MSG_MOD(extack, "Specified device does not exist");
+ return -ENODEV;
+ }
+ } else {
+ if (!exists) {
+ tcf_idr_cleanup(tn, parm->index);
+ NL_SET_ERR_MSG_MOD(extack, "No ifindex for new mirr action");
return -EINVAL;
}
+ dev = NULL;
+ }
+
+ if (!exists) {
ret = tcf_idr_create(tn, parm->index, est, a,
&act_mirred_ops, bind, true);
if (ret) {
tcf_idr_cleanup(tn, parm->index);
+ if (dev)
+ dev_put(dev);
return ret;
}
ret = ACT_P_CREATED;
} else if (!ovr) {
tcf_idr_release(*a, bind);
+ if (dev)
+ dev_put(dev);
return -EEXIST;
}
m = to_mirred(*a);
@@ -163,13 +181,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
m->tcf_action = parm->action;
m->tcfm_eaction = parm->eaction;
- if (parm->ifindex) {
- dev = dev_get_by_index(net, parm->ifindex);
- if (!dev) {
- spin_unlock_bh(&m->tcf_lock);
- tcf_idr_release(*a, bind);
- return -ENODEV;
- }
+ if (dev) {
mac_header_xmit = dev_is_mac_header_xmit(dev);
rcu_swap_protected(m->tcfm_dev, dev,
lockdep_is_held(&m->tcf_lock));
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/1] net: sched: fix cleanup NULL pointer exception in act_mirr
2019-03-20 18:12 [PATCH net 1/1] net: sched: fix cleanup NULL pointer exception in act_mirr John Hurley
@ 2019-03-21 18:05 ` Cong Wang
2019-03-21 18:22 ` Jakub Kicinski
2019-03-21 20:37 ` David Miller
1 sibling, 1 reply; 6+ messages in thread
From: Cong Wang @ 2019-03-21 18:05 UTC (permalink / raw)
To: John Hurley
Cc: Jiri Pirko, David Miller, Linux Kernel Network Developers,
Vlad Buslov, oss-drivers
On Wed, Mar 20, 2019 at 11:12 AM John Hurley <john.hurley@netronome.com> wrote:
>
> A new mirred action is created by the tcf_mirred_init function. This
> contains a list head struct which is inserted into a global list on
> successful creation of a new action. However, after a creation, it is
> still possible to error out if the egress device does not exist. This
> calls the act_mirr cleanup function via __tcf_idr_release and
> __tcf_action_put. This cleanup function tries to delete the list entry
> which is as yet uninitialised, leading to a NULL pointer exception.
Hmm, good catch but can this be just fixed by initializing it before
taking the netdevice refcnt? Like this:
@@ -163,6 +163,9 @@ static int tcf_mirred_init(struct net *net, struct
nlattr *nla,
m->tcf_action = parm->action;
m->tcfm_eaction = parm->eaction;
+ if (ret == ACT_P_CREATED)
+ INIT_LIST_HEAD(&m->tcfm_list);
+
if (parm->ifindex) {
dev = dev_get_by_index(net, parm->ifindex);
if (!dev) {
which is also much simpler.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/1] net: sched: fix cleanup NULL pointer exception in act_mirr
2019-03-21 18:05 ` Cong Wang
@ 2019-03-21 18:22 ` Jakub Kicinski
2019-03-21 18:26 ` Cong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2019-03-21 18:22 UTC (permalink / raw)
To: Cong Wang
Cc: John Hurley, Jiri Pirko, David Miller,
Linux Kernel Network Developers, Vlad Buslov, oss-drivers
On Thu, 21 Mar 2019 11:05:21 -0700, Cong Wang wrote:
> On Wed, Mar 20, 2019 at 11:12 AM John Hurley <john.hurley@netronome.com> wrote:
> >
> > A new mirred action is created by the tcf_mirred_init function. This
> > contains a list head struct which is inserted into a global list on
> > successful creation of a new action. However, after a creation, it is
> > still possible to error out if the egress device does not exist. This
> > calls the act_mirr cleanup function via __tcf_idr_release and
> > __tcf_action_put. This cleanup function tries to delete the list entry
> > which is as yet uninitialised, leading to a NULL pointer exception.
>
> Hmm, good catch but can this be just fixed by initializing it before
> taking the netdevice refcnt? Like this:
>
> @@ -163,6 +163,9 @@ static int tcf_mirred_init(struct net *net, struct
> nlattr *nla,
> m->tcf_action = parm->action;
> m->tcfm_eaction = parm->eaction;
>
> + if (ret == ACT_P_CREATED)
> + INIT_LIST_HEAD(&m->tcfm_list);
> +
> if (parm->ifindex) {
> dev = dev_get_by_index(net, parm->ifindex);
> if (!dev) {
>
> which is also much simpler.
That's the initial way John fixed it, but I asked him to go back to the
previous way this code was written.
I think having the parameters validated before any allocations happen
is less error prone, especially with the strange way actions get the
release call even when init failed. It's just a more reliable code
patter for actions' init callback.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/1] net: sched: fix cleanup NULL pointer exception in act_mirr
2019-03-21 18:22 ` Jakub Kicinski
@ 2019-03-21 18:26 ` Cong Wang
2019-03-21 18:40 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2019-03-21 18:26 UTC (permalink / raw)
To: Jakub Kicinski
Cc: John Hurley, Jiri Pirko, David Miller,
Linux Kernel Network Developers, Vlad Buslov, oss-drivers
On Thu, Mar 21, 2019 at 11:22 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Thu, 21 Mar 2019 11:05:21 -0700, Cong Wang wrote:
> > On Wed, Mar 20, 2019 at 11:12 AM John Hurley <john.hurley@netronome.com> wrote:
> > >
> > > A new mirred action is created by the tcf_mirred_init function. This
> > > contains a list head struct which is inserted into a global list on
> > > successful creation of a new action. However, after a creation, it is
> > > still possible to error out if the egress device does not exist. This
> > > calls the act_mirr cleanup function via __tcf_idr_release and
> > > __tcf_action_put. This cleanup function tries to delete the list entry
> > > which is as yet uninitialised, leading to a NULL pointer exception.
> >
> > Hmm, good catch but can this be just fixed by initializing it before
> > taking the netdevice refcnt? Like this:
> >
> > @@ -163,6 +163,9 @@ static int tcf_mirred_init(struct net *net, struct
> > nlattr *nla,
> > m->tcf_action = parm->action;
> > m->tcfm_eaction = parm->eaction;
> >
> > + if (ret == ACT_P_CREATED)
> > + INIT_LIST_HEAD(&m->tcfm_list);
> > +
> > if (parm->ifindex) {
> > dev = dev_get_by_index(net, parm->ifindex);
> > if (!dev) {
> >
> > which is also much simpler.
>
> That's the initial way John fixed it, but I asked him to go back to the
> previous way this code was written.
>
> I think having the parameters validated before any allocations happen
> is less error prone, especially with the strange way actions get the
> release call even when init failed. It's just a more reliable code
> patter for actions' init callback.
The point is a simpler version is better for -net and -stable,
and for review too.
You can always refactor it for net-next if you feel necessary.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/1] net: sched: fix cleanup NULL pointer exception in act_mirr
2019-03-21 18:26 ` Cong Wang
@ 2019-03-21 18:40 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2019-03-21 18:40 UTC (permalink / raw)
To: Cong Wang
Cc: John Hurley, Jiri Pirko, David Miller,
Linux Kernel Network Developers, Vlad Buslov, oss-drivers
On Thu, 21 Mar 2019 11:26:56 -0700, Cong Wang wrote:
> On Thu, Mar 21, 2019 at 11:22 AM Jakub Kicinski wrote:
> > On Thu, 21 Mar 2019 11:05:21 -0700, Cong Wang wrote:
> > > On Wed, Mar 20, 2019 at 11:12 AM John Hurley wrote:
> > > >
> > > > A new mirred action is created by the tcf_mirred_init function. This
> > > > contains a list head struct which is inserted into a global list on
> > > > successful creation of a new action. However, after a creation, it is
> > > > still possible to error out if the egress device does not exist. This
> > > > calls the act_mirr cleanup function via __tcf_idr_release and
> > > > __tcf_action_put. This cleanup function tries to delete the list entry
> > > > which is as yet uninitialised, leading to a NULL pointer exception.
> > >
> > > Hmm, good catch but can this be just fixed by initializing it before
> > > taking the netdevice refcnt? Like this:
> > >
> > > @@ -163,6 +163,9 @@ static int tcf_mirred_init(struct net *net, struct
> > > nlattr *nla,
> > > m->tcf_action = parm->action;
> > > m->tcfm_eaction = parm->eaction;
> > >
> > > + if (ret == ACT_P_CREATED)
> > > + INIT_LIST_HEAD(&m->tcfm_list);
> > > +
> > > if (parm->ifindex) {
> > > dev = dev_get_by_index(net, parm->ifindex);
> > > if (!dev) {
> > >
> > > which is also much simpler.
> >
> > That's the initial way John fixed it, but I asked him to go back to the
> > previous way this code was written.
> >
> > I think having the parameters validated before any allocations happen
> > is less error prone, especially with the strange way actions get the
> > release call even when init failed. It's just a more reliable code
> > patter for actions' init callback.
>
> The point is a simpler version is better for -net and -stable,
> and for review too.
>
> You can always refactor it for net-next if you feel necessary.
I personally prefer to push a slightly bigger fix (30 lines of change
is hardly large after all) if it means we can have the same code in
net-next as in LTS kernels (4.19 in this case). Especially for
relatively stable code.
But yeah, I was on the fence too, so given your preference we can go
back to init head.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/1] net: sched: fix cleanup NULL pointer exception in act_mirr
2019-03-20 18:12 [PATCH net 1/1] net: sched: fix cleanup NULL pointer exception in act_mirr John Hurley
2019-03-21 18:05 ` Cong Wang
@ 2019-03-21 20:37 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2019-03-21 20:37 UTC (permalink / raw)
To: john.hurley; +Cc: jiri, xiyou.wangcong, netdev, vladbu, oss-drivers
I prefer the two-liner suggested by Cong as well.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-21 20:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-20 18:12 [PATCH net 1/1] net: sched: fix cleanup NULL pointer exception in act_mirr John Hurley
2019-03-21 18:05 ` Cong Wang
2019-03-21 18:22 ` Jakub Kicinski
2019-03-21 18:26 ` Cong Wang
2019-03-21 18:40 ` Jakub Kicinski
2019-03-21 20:37 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).