| Message ID | 20230707075342.2463015-1-william.xuanziyang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] can: raw: fix receiver memory leak | expand |
Hello William, On 07.07.23 09:53, Ziyang Xuan wrote: > Got kmemleak errors with the following ltp can_filter testcase: > > for ((i=1; i<=100; i++)) > do > ./can_filter & > sleep 0.1 > done > > ============================================================== > [<00000000db4a4943>] can_rx_register+0x147/0x360 [can] > [<00000000a289549d>] raw_setsockopt+0x5ef/0x853 [can_raw] > [<000000006d3d9ebd>] __sys_setsockopt+0x173/0x2c0 > [<00000000407dbfec>] __x64_sys_setsockopt+0x61/0x70 > [<00000000fd468496>] do_syscall_64+0x33/0x40 > [<00000000b7e47d51>] entry_SYSCALL_64_after_hwframe+0x61/0xc6 > > It's a bug in the concurrent scenario of unregister_netdevice_many() > and raw_release() as following: > > cpu0 cpu1 > unregister_netdevice_many(can_dev) > unlist_netdevice(can_dev) // dev_get_by_index() return NULL after this > net_set_todo(can_dev) > raw_release(can_socket) > dev = dev_get_by_index(, ro->ifindex); // dev == NULL > if (dev) { // receivers in dev_rcv_lists not free because dev is NULL > raw_disable_allfilters(, dev, ); > dev_put(dev); > } > ... > ro->bound = 0; > ... > > call_netdevice_notifiers(NETDEV_UNREGISTER, ) > raw_notify(, NETDEV_UNREGISTER, ) > if (ro->bound) // invalid because ro->bound has been set 0 > raw_disable_allfilters(, dev, ); // receivers in dev_rcv_lists will never be freed > > Add a net_device pointer member in struct raw_sock to record bound can_dev, > and use rtnl_lock to serialize raw_socket members between raw_bind(), raw_release(), > raw_setsockopt() and raw_notify(). Use ro->dev to decide whether to free receivers in > dev_rcv_lists. > > Fixes: 8d0caedb7596 ("can: bcm/raw/isotp: use per module netdevice notifier") > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > --- > v2: > - Fix the case for a bound socket for "all" CAN interfaces (ifindex == 0) in raw_bind(). > --- > net/can/raw.c | 61 ++++++++++++++++++++++----------------------------- > 1 file changed, 26 insertions(+), 35 deletions(-) > > diff --git a/net/can/raw.c b/net/can/raw.c > index 15c79b079184..7078821f35e0 100644 > --- a/net/can/raw.c > +++ b/net/can/raw.c > @@ -84,6 +84,7 @@ struct raw_sock { > struct sock sk; > int bound; > int ifindex; > + struct net_device *dev; > struct list_head notifier; > int loopback; > int recv_own_msgs; > @@ -277,7 +278,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg, > if (!net_eq(dev_net(dev), sock_net(sk))) > return; > > - if (ro->ifindex != dev->ifindex) > + if (ro->dev != dev) > return; > > switch (msg) { > @@ -292,6 +293,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg, > > ro->ifindex = 0; > ro->bound = 0; > + ro->dev = NULL; > ro->count = 0; > release_sock(sk); > > @@ -337,6 +339,7 @@ static int raw_init(struct sock *sk) > > ro->bound = 0; > ro->ifindex = 0; > + ro->dev = NULL; > > /* set default filter to single entry dfilter */ > ro->dfilter.can_id = 0; > @@ -385,19 +388,13 @@ static int raw_release(struct socket *sock) > > lock_sock(sk); > > + rtnl_lock(); > /* remove current filters & unregister */ > if (ro->bound) { > - if (ro->ifindex) { > - struct net_device *dev; > - > - dev = dev_get_by_index(sock_net(sk), ro->ifindex); > - if (dev) { > - raw_disable_allfilters(dev_net(dev), dev, sk); > - dev_put(dev); > - } > - } else { > + if (ro->dev) > + raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk); > + else > raw_disable_allfilters(sock_net(sk), NULL, sk); > - } > } > > if (ro->count > 1) > @@ -405,8 +402,10 @@ static int raw_release(struct socket *sock) > > ro->ifindex = 0; > ro->bound = 0; > + ro->dev = NULL; > ro->count = 0; > free_percpu(ro->uniq); > + rtnl_unlock(); > > sock_orphan(sk); > sock->sk = NULL; > @@ -422,6 +421,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) > struct sockaddr_can *addr = (struct sockaddr_can *)uaddr; > struct sock *sk = sock->sk; > struct raw_sock *ro = raw_sk(sk); > + struct net_device *dev = NULL; > int ifindex; > int err = 0; > int notify_enetdown = 0; > @@ -431,14 +431,13 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) > if (addr->can_family != AF_CAN) > return -EINVAL; > > + rtnl_lock(); > lock_sock(sk); > > if (ro->bound && addr->can_ifindex == ro->ifindex) > goto out; > > if (addr->can_ifindex) { > - struct net_device *dev; > - > dev = dev_get_by_index(sock_net(sk), addr->can_ifindex); > if (!dev) { > err = -ENODEV; > @@ -465,28 +464,23 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) > } > > if (!err) { > + /* unregister old filters */ > if (ro->bound) { > - /* unregister old filters */ Please move this comment back as we only unregister old filters when the socket is bound. > - if (ro->ifindex) { > - struct net_device *dev; > - > - dev = dev_get_by_index(sock_net(sk), > - ro->ifindex); > - if (dev) { > - raw_disable_allfilters(dev_net(dev), > - dev, sk); > - dev_put(dev); > - } > - } else { > + if (ro->dev) > + raw_disable_allfilters(dev_net(ro->dev), > + ro->dev, sk); > + else > raw_disable_allfilters(sock_net(sk), NULL, sk); > - } > } > ro->ifindex = ifindex; > + Why did you add an additional empty line here? Please remove. > ro->bound = 1; > + ro->dev = dev; > } > > out: > release_sock(sk); > + rtnl_unlock(); > > if (notify_enetdown) { > sk->sk_err = ENETDOWN; > @@ -553,9 +547,9 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, > rtnl_lock(); > lock_sock(sk); > > - if (ro->bound && ro->ifindex) { > - dev = dev_get_by_index(sock_net(sk), ro->ifindex); > - if (!dev) { > + dev = ro->dev; > + if (ro->bound && dev) { > + if (dev->reg_state != NETREG_REGISTERED) { > if (count > 1) > kfree(filter); > err = -ENODEV; > @@ -596,7 +590,6 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, > ro->count = count; > > out_fil: > - dev_put(dev); > release_sock(sk); > rtnl_unlock(); > > @@ -614,9 +607,9 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, > rtnl_lock(); > lock_sock(sk); > > - if (ro->bound && ro->ifindex) { > - dev = dev_get_by_index(sock_net(sk), ro->ifindex); > - if (!dev) { > + dev = ro->dev; > + if (ro->bound && dev) { > + if (dev->reg_state != NETREG_REGISTERED) { > err = -ENODEV; > goto out_err; > } > @@ -627,7 +620,6 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, > /* (try to) register the new err_mask */ > err = raw_enable_errfilter(sock_net(sk), dev, sk, > err_mask); > - And here you removed an empty line? Please omit such mix of fixing a bug and change the coding style. > if (err) > goto out_err; > > @@ -640,7 +632,6 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, > ro->err_mask = err_mask; > > out_err: > - dev_put(dev); > release_sock(sk); > rtnl_unlock(); > The rest looks fine now, many thanks! It also reduces the code size. When you send the v3 you can add these tags: Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net> Best regards, Oliver
> Hello William, > > On 07.07.23 09:53, Ziyang Xuan wrote: >> Got kmemleak errors with the following ltp can_filter testcase: >> >> for ((i=1; i<=100; i++)) >> do >> ./can_filter & >> sleep 0.1 >> done >> >> ============================================================== >> [<00000000db4a4943>] can_rx_register+0x147/0x360 [can] >> [<00000000a289549d>] raw_setsockopt+0x5ef/0x853 [can_raw] >> [<000000006d3d9ebd>] __sys_setsockopt+0x173/0x2c0 >> [<00000000407dbfec>] __x64_sys_setsockopt+0x61/0x70 >> [<00000000fd468496>] do_syscall_64+0x33/0x40 >> [<00000000b7e47d51>] entry_SYSCALL_64_after_hwframe+0x61/0xc6 >> >> It's a bug in the concurrent scenario of unregister_netdevice_many() >> and raw_release() as following: >> >> cpu0 cpu1 >> unregister_netdevice_many(can_dev) >> unlist_netdevice(can_dev) // dev_get_by_index() return NULL after this >> net_set_todo(can_dev) >> raw_release(can_socket) >> dev = dev_get_by_index(, ro->ifindex); // dev == NULL >> if (dev) { // receivers in dev_rcv_lists not free because dev is NULL >> raw_disable_allfilters(, dev, ); >> dev_put(dev); >> } >> ... >> ro->bound = 0; >> ... >> >> call_netdevice_notifiers(NETDEV_UNREGISTER, ) >> raw_notify(, NETDEV_UNREGISTER, ) >> if (ro->bound) // invalid because ro->bound has been set 0 >> raw_disable_allfilters(, dev, ); // receivers in dev_rcv_lists will never be freed >> >> Add a net_device pointer member in struct raw_sock to record bound can_dev, >> and use rtnl_lock to serialize raw_socket members between raw_bind(), raw_release(), >> raw_setsockopt() and raw_notify(). Use ro->dev to decide whether to free receivers in >> dev_rcv_lists. >> >> Fixes: 8d0caedb7596 ("can: bcm/raw/isotp: use per module netdevice notifier") >> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >> --- >> v2: >> - Fix the case for a bound socket for "all" CAN interfaces (ifindex == 0) in raw_bind(). >> --- >> net/can/raw.c | 61 ++++++++++++++++++++++----------------------------- >> 1 file changed, 26 insertions(+), 35 deletions(-) >> >> diff --git a/net/can/raw.c b/net/can/raw.c >> index 15c79b079184..7078821f35e0 100644 >> --- a/net/can/raw.c >> +++ b/net/can/raw.c >> @@ -84,6 +84,7 @@ struct raw_sock { >> struct sock sk; >> int bound; >> int ifindex; >> + struct net_device *dev; >> struct list_head notifier; >> int loopback; >> int recv_own_msgs; >> @@ -277,7 +278,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg, >> if (!net_eq(dev_net(dev), sock_net(sk))) >> return; >> - if (ro->ifindex != dev->ifindex) >> + if (ro->dev != dev) >> return; >> switch (msg) { >> @@ -292,6 +293,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg, >> ro->ifindex = 0; >> ro->bound = 0; >> + ro->dev = NULL; >> ro->count = 0; >> release_sock(sk); >> @@ -337,6 +339,7 @@ static int raw_init(struct sock *sk) >> ro->bound = 0; >> ro->ifindex = 0; >> + ro->dev = NULL; >> /* set default filter to single entry dfilter */ >> ro->dfilter.can_id = 0; >> @@ -385,19 +388,13 @@ static int raw_release(struct socket *sock) >> lock_sock(sk); >> + rtnl_lock(); >> /* remove current filters & unregister */ >> if (ro->bound) { >> - if (ro->ifindex) { >> - struct net_device *dev; >> - >> - dev = dev_get_by_index(sock_net(sk), ro->ifindex); >> - if (dev) { >> - raw_disable_allfilters(dev_net(dev), dev, sk); >> - dev_put(dev); >> - } >> - } else { >> + if (ro->dev) >> + raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk); >> + else >> raw_disable_allfilters(sock_net(sk), NULL, sk); >> - } >> } >> if (ro->count > 1) >> @@ -405,8 +402,10 @@ static int raw_release(struct socket *sock) >> ro->ifindex = 0; >> ro->bound = 0; >> + ro->dev = NULL; >> ro->count = 0; >> free_percpu(ro->uniq); >> + rtnl_unlock(); >> sock_orphan(sk); >> sock->sk = NULL; >> @@ -422,6 +421,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) >> struct sockaddr_can *addr = (struct sockaddr_can *)uaddr; >> struct sock *sk = sock->sk; >> struct raw_sock *ro = raw_sk(sk); >> + struct net_device *dev = NULL; >> int ifindex; >> int err = 0; >> int notify_enetdown = 0; >> @@ -431,14 +431,13 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) >> if (addr->can_family != AF_CAN) >> return -EINVAL; >> + rtnl_lock(); >> lock_sock(sk); >> if (ro->bound && addr->can_ifindex == ro->ifindex) >> goto out; >> if (addr->can_ifindex) { >> - struct net_device *dev; >> - >> dev = dev_get_by_index(sock_net(sk), addr->can_ifindex); >> if (!dev) { >> err = -ENODEV; >> @@ -465,28 +464,23 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) >> } >> if (!err) { >> + /* unregister old filters */ >> if (ro->bound) { >> - /* unregister old filters */ > > Please move this comment back as we only unregister old filters when the socket is bound. > >> - if (ro->ifindex) { >> - struct net_device *dev; >> - >> - dev = dev_get_by_index(sock_net(sk), >> - ro->ifindex); >> - if (dev) { >> - raw_disable_allfilters(dev_net(dev), >> - dev, sk); >> - dev_put(dev); >> - } >> - } else { >> + if (ro->dev) >> + raw_disable_allfilters(dev_net(ro->dev), >> + ro->dev, sk); >> + else >> raw_disable_allfilters(sock_net(sk), NULL, sk); >> - } >> } >> ro->ifindex = ifindex; >> + > > Why did you add an additional empty line here? > Please remove. > >> ro->bound = 1; >> + ro->dev = dev; >> } >> out: >> release_sock(sk); >> + rtnl_unlock(); >> if (notify_enetdown) { >> sk->sk_err = ENETDOWN; >> @@ -553,9 +547,9 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, >> rtnl_lock(); >> lock_sock(sk); >> - if (ro->bound && ro->ifindex) { >> - dev = dev_get_by_index(sock_net(sk), ro->ifindex); >> - if (!dev) { >> + dev = ro->dev; >> + if (ro->bound && dev) { >> + if (dev->reg_state != NETREG_REGISTERED) { >> if (count > 1) >> kfree(filter); >> err = -ENODEV; >> @@ -596,7 +590,6 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, >> ro->count = count; >> out_fil: >> - dev_put(dev); >> release_sock(sk); >> rtnl_unlock(); >> @@ -614,9 +607,9 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, >> rtnl_lock(); >> lock_sock(sk); >> - if (ro->bound && ro->ifindex) { >> - dev = dev_get_by_index(sock_net(sk), ro->ifindex); >> - if (!dev) { >> + dev = ro->dev; >> + if (ro->bound && dev) { >> + if (dev->reg_state != NETREG_REGISTERED) { >> err = -ENODEV; >> goto out_err; >> } >> @@ -627,7 +620,6 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, >> /* (try to) register the new err_mask */ >> err = raw_enable_errfilter(sock_net(sk), dev, sk, >> err_mask); >> - > > And here you removed an empty line? > > Please omit such mix of fixing a bug and change the coding style. > >> if (err) >> goto out_err; >> @@ -640,7 +632,6 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, >> ro->err_mask = err_mask; >> out_err: >> - dev_put(dev); >> release_sock(sk); >> rtnl_unlock(); >> > > The rest looks fine now, many thanks! > It also reduces the code size. > > When you send the v3 you can add these tags: > > Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net> > Acked-by: Oliver Hartkopp <socketcan@hartkopp.net> > OK, Thank you for your comments. > Best regards, > Oliver > > .
diff --git a/net/can/raw.c b/net/can/raw.c index 15c79b079184..7078821f35e0 100644 --- a/net/can/raw.c +++ b/net/can/raw.c @@ -84,6 +84,7 @@ struct raw_sock { struct sock sk; int bound; int ifindex; + struct net_device *dev; struct list_head notifier; int loopback; int recv_own_msgs; @@ -277,7 +278,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg, if (!net_eq(dev_net(dev), sock_net(sk))) return; - if (ro->ifindex != dev->ifindex) + if (ro->dev != dev) return; switch (msg) { @@ -292,6 +293,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg, ro->ifindex = 0; ro->bound = 0; + ro->dev = NULL; ro->count = 0; release_sock(sk); @@ -337,6 +339,7 @@ static int raw_init(struct sock *sk) ro->bound = 0; ro->ifindex = 0; + ro->dev = NULL; /* set default filter to single entry dfilter */ ro->dfilter.can_id = 0; @@ -385,19 +388,13 @@ static int raw_release(struct socket *sock) lock_sock(sk); + rtnl_lock(); /* remove current filters & unregister */ if (ro->bound) { - if (ro->ifindex) { - struct net_device *dev; - - dev = dev_get_by_index(sock_net(sk), ro->ifindex); - if (dev) { - raw_disable_allfilters(dev_net(dev), dev, sk); - dev_put(dev); - } - } else { + if (ro->dev) + raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk); + else raw_disable_allfilters(sock_net(sk), NULL, sk); - } } if (ro->count > 1) @@ -405,8 +402,10 @@ static int raw_release(struct socket *sock) ro->ifindex = 0; ro->bound = 0; + ro->dev = NULL; ro->count = 0; free_percpu(ro->uniq); + rtnl_unlock(); sock_orphan(sk); sock->sk = NULL; @@ -422,6 +421,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) struct sockaddr_can *addr = (struct sockaddr_can *)uaddr; struct sock *sk = sock->sk; struct raw_sock *ro = raw_sk(sk); + struct net_device *dev = NULL; int ifindex; int err = 0; int notify_enetdown = 0; @@ -431,14 +431,13 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) if (addr->can_family != AF_CAN) return -EINVAL; + rtnl_lock(); lock_sock(sk); if (ro->bound && addr->can_ifindex == ro->ifindex) goto out; if (addr->can_ifindex) { - struct net_device *dev; - dev = dev_get_by_index(sock_net(sk), addr->can_ifindex); if (!dev) { err = -ENODEV; @@ -465,28 +464,23 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) } if (!err) { + /* unregister old filters */ if (ro->bound) { - /* unregister old filters */ - if (ro->ifindex) { - struct net_device *dev; - - dev = dev_get_by_index(sock_net(sk), - ro->ifindex); - if (dev) { - raw_disable_allfilters(dev_net(dev), - dev, sk); - dev_put(dev); - } - } else { + if (ro->dev) + raw_disable_allfilters(dev_net(ro->dev), + ro->dev, sk); + else raw_disable_allfilters(sock_net(sk), NULL, sk); - } } ro->ifindex = ifindex; + ro->bound = 1; + ro->dev = dev; } out: release_sock(sk); + rtnl_unlock(); if (notify_enetdown) { sk->sk_err = ENETDOWN; @@ -553,9 +547,9 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, rtnl_lock(); lock_sock(sk); - if (ro->bound && ro->ifindex) { - dev = dev_get_by_index(sock_net(sk), ro->ifindex); - if (!dev) { + dev = ro->dev; + if (ro->bound && dev) { + if (dev->reg_state != NETREG_REGISTERED) { if (count > 1) kfree(filter); err = -ENODEV; @@ -596,7 +590,6 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, ro->count = count; out_fil: - dev_put(dev); release_sock(sk); rtnl_unlock(); @@ -614,9 +607,9 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, rtnl_lock(); lock_sock(sk); - if (ro->bound && ro->ifindex) { - dev = dev_get_by_index(sock_net(sk), ro->ifindex); - if (!dev) { + dev = ro->dev; + if (ro->bound && dev) { + if (dev->reg_state != NETREG_REGISTERED) { err = -ENODEV; goto out_err; } @@ -627,7 +620,6 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, /* (try to) register the new err_mask */ err = raw_enable_errfilter(sock_net(sk), dev, sk, err_mask); - if (err) goto out_err; @@ -640,7 +632,6 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, ro->err_mask = err_mask; out_err: - dev_put(dev); release_sock(sk); rtnl_unlock();
Got kmemleak errors with the following ltp can_filter testcase: for ((i=1; i<=100; i++)) do ./can_filter & sleep 0.1 done ============================================================== [<00000000db4a4943>] can_rx_register+0x147/0x360 [can] [<00000000a289549d>] raw_setsockopt+0x5ef/0x853 [can_raw] [<000000006d3d9ebd>] __sys_setsockopt+0x173/0x2c0 [<00000000407dbfec>] __x64_sys_setsockopt+0x61/0x70 [<00000000fd468496>] do_syscall_64+0x33/0x40 [<00000000b7e47d51>] entry_SYSCALL_64_after_hwframe+0x61/0xc6 It's a bug in the concurrent scenario of unregister_netdevice_many() and raw_release() as following: cpu0 cpu1 unregister_netdevice_many(can_dev) unlist_netdevice(can_dev) // dev_get_by_index() return NULL after this net_set_todo(can_dev) raw_release(can_socket) dev = dev_get_by_index(, ro->ifindex); // dev == NULL if (dev) { // receivers in dev_rcv_lists not free because dev is NULL raw_disable_allfilters(, dev, ); dev_put(dev); } ... ro->bound = 0; ... call_netdevice_notifiers(NETDEV_UNREGISTER, ) raw_notify(, NETDEV_UNREGISTER, ) if (ro->bound) // invalid because ro->bound has been set 0 raw_disable_allfilters(, dev, ); // receivers in dev_rcv_lists will never be freed Add a net_device pointer member in struct raw_sock to record bound can_dev, and use rtnl_lock to serialize raw_socket members between raw_bind(), raw_release(), raw_setsockopt() and raw_notify(). Use ro->dev to decide whether to free receivers in dev_rcv_lists. Fixes: 8d0caedb7596 ("can: bcm/raw/isotp: use per module netdevice notifier") Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- v2: - Fix the case for a bound socket for "all" CAN interfaces (ifindex == 0) in raw_bind(). --- net/can/raw.c | 61 ++++++++++++++++++++++----------------------------- 1 file changed, 26 insertions(+), 35 deletions(-)