Commit ee8b94c8 authored by Ziyang Xuan's avatar Ziyang Xuan Committed by Marc Kleine-Budde
Browse files

can: raw: fix receiver memory leak 



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: 8d0caedb ("can: bcm/raw/isotp: use per module netdevice notifier")
Reviewed-by: default avatarOliver Hartkopp <socketcan@hartkopp.net>
Acked-by: default avatarOliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: default avatarZiyang Xuan <william.xuanziyang@huawei.com>
Link: https://lore.kernel.org/all/20230711011737.1969582-1-william.xuanziyang@huawei.com


Cc: stable@vger.kernel.org
Signed-off-by: default avatarMarc Kleine-Budde <mkl@pengutronix.de>
parent 0dd1805f

net/can/raw.c

+24 −33
Original line number Diff line number Diff line
@@ -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,28 +388,24 @@ 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)

		kfree(ro->filter);



	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;
@@ -467,26 +466,20 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) 
	if (!err) {

		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 +546,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 +589,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 +606,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;

			}
@@ -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();