diff mbox series

[net] can: raw: fix receiver memory leak

Message ID 20230705092543.648022-1-william.xuanziyang@huawei.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] can: raw: fix receiver memory leak | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers fail 2 blamed authors not CCed: penguin-kernel@i-love.sakura.ne.jp tkhai@ya.ru; 2 maintainers not CCed: penguin-kernel@i-love.sakura.ne.jp tkhai@ya.ru
netdev/build_clang fail Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 166 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Ziyang Xuan (William) July 5, 2023, 9:25 a.m. UTC
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>
---
 net/can/raw.c | 63 ++++++++++++++++++++++-----------------------------
 1 file changed, 27 insertions(+), 36 deletions(-)

Comments

Oliver Hartkopp July 6, 2023, 11:55 a.m. UTC | #1
Hello Ziyang Xuan,

thanks for your patch and the found inconsistency!

The ro->ifindex value might be zero even on a bound CAN_RAW socket which 
results in the use of a common filter for all CAN interfaces, see below ...

On 2023-07-05 11:25, Ziyang Xuan wrote:

(..)

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

This would be ok for raw_notify().

> @@ -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;

This would be ok too.

> @@ -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)
> +	if (ro->bound && ro->dev && addr->can_ifindex == ro->dev->ifindex)

But this is wrong as the case for a bound socket for "all" CAN 
interfaces (ifindex == 0) is not considered.

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

Would it also fix the issue when just adding the rtnl_locks to 
raw_bind() and raw_release() as suggested by you?

Many thanks,
Oliver

>   
>   	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();
>
Ziyang Xuan (William) July 6, 2023, 12:48 p.m. UTC | #2
> Hello Ziyang Xuan,
> 
> thanks for your patch and the found inconsistency!
> 
> The ro->ifindex value might be zero even on a bound CAN_RAW socket which results in the use of a common filter for all CAN interfaces, see below ...
> 
> On 2023-07-05 11:25, Ziyang Xuan wrote:
> 
> (..)
> 
>> @@ -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);
>>   
> 
> This would be ok for raw_notify().
> 
>> @@ -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;
> 
> This would be ok too.
> 
>> @@ -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)
>> +    if (ro->bound && ro->dev && addr->can_ifindex == ro->dev->ifindex)
> 
> But this is wrong as the case for a bound socket for "all" CAN interfaces (ifindex == 0) is not considered.

Yes, here need not modification. I will fix it in v2. Thanks!

> 
>>           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();
> 
> Would it also fix the issue when just adding the rtnl_locks to raw_bind() and raw_release() as suggested by you?

This patch just add rtnl_lock in raw_bind() and raw_release(). raw_setsockopt() has rtnl_lock before this. raw_notify()
is under rtnl_lock. My patch has been tested and solved the issue before send. I don't know if it answered your doubts.

Thanks.
William Xuan

> 
> Many thanks,
> Oliver
> 
>>         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();
>>   
> .
Oliver Hartkopp July 6, 2023, 6:29 p.m. UTC | #3
On 06.07.23 14:48, Ziyang Xuan (William) wrote:

(..)

>>>        }
>>>       out:
>>>        release_sock(sk);
>>> +    rtnl_unlock();
>>
>> Would it also fix the issue when just adding the rtnl_locks to raw_bind() and raw_release() as suggested by you?
> 
> This patch just add rtnl_lock in raw_bind() and raw_release(). raw_setsockopt() has rtnl_lock before this. raw_notify()
> is under rtnl_lock. My patch has been tested and solved the issue before send. I don't know if it answered your doubts.

My question was whether adding rtnl_locks to raw_bind() and 
raw_release() would be enough to fix the issue.

Without introducing the additional ro->dev element!?

Best regards,
Oliver
Ziyang Xuan (William) July 7, 2023, 1:59 a.m. UTC | #4
> On 06.07.23 14:48, Ziyang Xuan (William) wrote:
> 
> (..)
> 
>>>>        }
>>>>       out:
>>>>        release_sock(sk);
>>>> +    rtnl_unlock();
>>>
>>> Would it also fix the issue when just adding the rtnl_locks to raw_bind() and raw_release() as suggested by you?
>>
>> This patch just add rtnl_lock in raw_bind() and raw_release(). raw_setsockopt() has rtnl_lock before this. raw_notify()
>> is under rtnl_lock. My patch has been tested and solved the issue before send. I don't know if it answered your doubts.
> 
> My question was whether adding rtnl_locks to raw_bind() and raw_release() would be enough to fix the issue.
> 
> Without introducing the additional ro->dev element!?

Understand. Just add rtnl_lock to raw_bind() and raw_release() can not fix the issue. I tested.

We should understand that unregister a net device is divided into two stages generally.
Fistly, call unregister_netdevice_many() to remove net_dev from device list and add
net_dev to net_todo_list. Secondly, free net_dev in netdev_run_todo().

In my issue. Firstly, unregister_netdevice_many() removed can_dev from device
list and added can_dev to net_todo_list. Then got NULL by dev_get_by_index()
and receivers in dev_rcv_lists would not be freed in raw_release().
After raw_release(), ro->bound would be set 0. When NETDEV_UNREGISTER event
arrived raw_notify(), receivers in dev_rcv_lists would not be freed too
because ro->bound was already 0. Thus receivers in dev_rcv_lists would be leaked.

             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;
						...

netdev_wait_allrefs_any()
  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


Thanks,
William Xuan

> 
> Best regards,
> Oliver
> .
Oliver Hartkopp July 7, 2023, 6:22 a.m. UTC | #5
On 07.07.23 03:59, Ziyang Xuan (William) wrote:
>> On 06.07.23 14:48, Ziyang Xuan (William) wrote:
>>
>> (..)
>>
>>>>>         }
>>>>>        out:
>>>>>         release_sock(sk);
>>>>> +    rtnl_unlock();
>>>>
>>>> Would it also fix the issue when just adding the rtnl_locks to raw_bind() and raw_release() as suggested by you?
>>>
>>> This patch just add rtnl_lock in raw_bind() and raw_release(). raw_setsockopt() has rtnl_lock before this. raw_notify()
>>> is under rtnl_lock. My patch has been tested and solved the issue before send. I don't know if it answered your doubts.
>>
>> My question was whether adding rtnl_locks to raw_bind() and raw_release() would be enough to fix the issue.
>>
>> Without introducing the additional ro->dev element!?
> 
> Understand. Just add rtnl_lock to raw_bind() and raw_release() can not fix the issue. I tested.
> 
> We should understand that unregister a net device is divided into two stages generally.
> Fistly, call unregister_netdevice_many() to remove net_dev from device list and add
> net_dev to net_todo_list. Secondly, free net_dev in netdev_run_todo().
> 
> In my issue. Firstly, unregister_netdevice_many() removed can_dev from device
> list and added can_dev to net_todo_list. Then got NULL by dev_get_by_index()
> and receivers in dev_rcv_lists would not be freed in raw_release().
> After raw_release(), ro->bound would be set 0. When NETDEV_UNREGISTER event
> arrived raw_notify(), receivers in dev_rcv_lists would not be freed too
> because ro->bound was already 0. Thus receivers in dev_rcv_lists would be leaked.

Thanks for the clarification and the testing!

I really assumed rtnl_lock would do this job and also protect the entire 
sequence starting with unregister_netdevice_many() !?!

Looking forward to the V2 patch then.

Many thanks,
Oliver

> 
>               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;
> 						...
> 
> netdev_wait_allrefs_any()
>    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
> 
> 
> Thanks,
> William Xuan
> 
>>
>> Best regards,
>> Oliver
>> .
diff mbox series

Patch

diff --git a/net/can/raw.c b/net/can/raw.c
index 15c79b079184..e767d356f695 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)
+	if (ro->bound && ro->dev && addr->can_ifindex == ro->dev->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();