[PATCH -net] netpoll: fix NULL pointer dereference in netpoll_cleanup

[PATCH -net] netpoll: fix NULL pointer dereference in netpoll_cleanup

[Nikolay Aleksandrov]

I've been hitting a NULL ptr deref while using netconsole because the
np->dev check and the pointer manipulation in netpoll_cleanup are done
without rtnl and the following sequence happens when having a netconsole
over a vlan and we remove the vlan while disabling the netconsole:
	CPU 1					CPU2
					removes vlan and calls the notifier
enters store_enabled(), calls
netdev_cleanup which checks np->dev
and then waits for rtnl
					executes the netconsole netdev
					release notifier making np->dev
					== NULL and releases rtnl
continues to dereference a member of
np->dev which at this point is == NULL

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 net/core/netpoll.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 2c637e9..569a185 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -1284,15 +1284,15 @@ EXPORT_SYMBOL_GPL(__netpoll_free_async);
 
 void netpoll_cleanup(struct netpoll *np)
 {
-	if (!np->dev)
-		return;
-
 	rtnl_lock();
+	if (!np->dev) {
+		rtnl_unlock();
+		return;
+	}
 	__netpoll_cleanup(np);
-	rtnl_unlock();
-
 	dev_put(np->dev);
 	np->dev = NULL;
+	rtnl_unlock();
 }
 EXPORT_SYMBOL(netpoll_cleanup);
 
-- 
1.8.1.4

Re: [PATCH -net] netpoll: fix NULL pointer dereference in netpoll_cleanup

[Nikolay Aleksandrov]

On 09/17/2013 04:12 PM, Nikolay Aleksandrov wrote:
> I've been hitting a NULL ptr deref while using netconsole because the
> np->dev check and the pointer manipulation in netpoll_cleanup are done
> without rtnl and the following sequence happens when having a netconsole
> over a vlan and we remove the vlan while disabling the netconsole:
> 	CPU 1					CPU2
> 					removes vlan and calls the notifier
> enters store_enabled(), calls
> netdev_cleanup which checks np->dev
> and then waits for rtnl
> 					executes the netconsole netdev
> 					release notifier making np->dev
> 					== NULL and releases rtnl
> continues to dereference a member of
> np->dev which at this point is == NULL
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> ---

Just FYI there seems to be a deadlock in netconsole as well:
rtnl -> nt->mutex in the notifier coupled with
nt->mutex -> rtnl in store_enabled()

I can re-post a patchset that fixes these together, because after this is
applied the NULL pointer dereference is not hit, but the deadlock is easily hit.
The deadlock was introduced in commit 7a163bfb7ce50895bbe67300ea610d31b9c09230
("netconsole: avoid a crash with multiple sysfs writers").

Nik

Re: [PATCH -net] netpoll: fix NULL pointer dereference in netpoll_cleanup

[Nikolay Aleksandrov]

On 09/17/2013 04:37 PM, Nikolay Aleksandrov wrote:
> commit 7a163bfb7ce50895bbe67300ea610d31b9c09230
> ("netconsole: avoid a crash with multiple sysfs writers")
I feel like I didn't explain this one well. The above commit actually
tries to fix the same issue AFAICT, and it can be reverted if/once my fix
is accepted, but I think to remove only the locking in the netconsole
netdev notifier to avoid the deadlock because the mutex lock is useful for
fixing a third bug in netconsole, that I intend to take care of once this
is sorted out.
So basically my next fix that takes care of the deadlock is dependent on
this patch, and I'll wait to see the feedback, if it gets accepted I'll
post the follow-up that takes care of the deadlock.

Cheers,
 Nik

Re: [PATCH -net] netpoll: fix NULL pointer dereference in netpoll_cleanup

[David Miller]

From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Tue, 17 Sep 2013 16:12:35 +0200

> @@ -1284,15 +1284,15 @@ EXPORT_SYMBOL_GPL(__netpoll_free_async);
>  
>  void netpoll_cleanup(struct netpoll *np)
>  {
> -	if (!np->dev)
> -		return;
> -
>  	rtnl_lock();
> +	if (!np->dev) {
> +		rtnl_unlock();
> +		return;
> +	}
>  	__netpoll_cleanup(np);
> -	rtnl_unlock();
> -
>  	dev_put(np->dev);
>  	np->dev = NULL;
> +	rtnl_unlock();
>  }
>  EXPORT_SYMBOL(netpoll_cleanup);

I know it seems arbitrary, but please do this like:

	lock();
	if (condition) {
		...
	}
	unlock();

rather than having multiple return/unlock code paths.

This style I am suggesting is easier to audit for locking problems.

Thanks.

Re: [PATCH -net] netpoll: fix NULL pointer dereference in netpoll_cleanup

[Joe Perches]

On Wed, 2013-09-18 at 12:15 -0400, David Miller wrote:
> From: Nikolay Aleksandrov <nikolay@redhat.com>
> Date: Tue, 17 Sep 2013 16:12:35 +0200
> 
> > @@ -1284,15 +1284,15 @@ EXPORT_SYMBOL_GPL(__netpoll_free_async);
> >  
> >  void netpoll_cleanup(struct netpoll *np)
> >  {
> > -     if (!np->dev)
> > -             return;
> > -
> >       rtnl_lock();
> > +     if (!np->dev) {
> > +             rtnl_unlock();
> > +             return;
> > +     }
> >       __netpoll_cleanup(np);
> > -     rtnl_unlock();
> > -
> >       dev_put(np->dev);
> >       np->dev = NULL;
> > +     rtnl_unlock();
> >  }
> >  EXPORT_SYMBOL(netpoll_cleanup);
> 
> I know it seems arbitrary, but please do this like:
> 
>         lock();
>         if (condition) {
>                 ...
>         }
>         unlock();
> 
> rather than having multiple return/unlock code paths.
> 
> This style I am suggesting is easier to audit for locking problems.

Another alternative is:

	lock();

	if (!condition)
		goto out;

	...

out:
	unlock();
	return ...;
}

Re: [PATCH -net] netpoll: fix NULL pointer dereference in netpoll_cleanup

[Nikolay Aleksandrov]

On 09/18/2013 06:15 PM, David Miller wrote:
> From: Nikolay Aleksandrov <nikolay@redhat.com>
> Date: Tue, 17 Sep 2013 16:12:35 +0200
> 
>> @@ -1284,15 +1284,15 @@ EXPORT_SYMBOL_GPL(__netpoll_free_async);
>>  
>>  void netpoll_cleanup(struct netpoll *np)
>>  {
>> -	if (!np->dev)
>> -		return;
>> -
>>  	rtnl_lock();
>> +	if (!np->dev) {
>> +		rtnl_unlock();
>> +		return;
>> +	}
>>  	__netpoll_cleanup(np);
>> -	rtnl_unlock();
>> -
>>  	dev_put(np->dev);
>>  	np->dev = NULL;
>> +	rtnl_unlock();
>>  }
>>  EXPORT_SYMBOL(netpoll_cleanup);
> 
> I know it seems arbitrary, but please do this like:
> 
> 	lock();
> 	if (condition) {
> 		...
> 	}
> 	unlock();
> 
> rather than having multiple return/unlock code paths.
> 
> This style I am suggesting is easier to audit for locking problems.
> 
> Thanks.
> 
Got it, I'll edit it and re-post.

Thanks,
 Nik

Re: [PATCH -net] netpoll: fix NULL pointer dereference in netpoll_cleanup

[Nikolay Aleksandrov]

On 09/18/2013 06:25 PM, Joe Perches wrote:
> On Wed, 2013-09-18 at 12:15 -0400, David Miller wrote:
>> From: Nikolay Aleksandrov <nikolay@redhat.com>
>> Date: Tue, 17 Sep 2013 16:12:35 +0200
>>
>>> @@ -1284,15 +1284,15 @@ EXPORT_SYMBOL_GPL(__netpoll_free_async);
>>>  
>>>  void netpoll_cleanup(struct netpoll *np)
>>>  {
>>> -     if (!np->dev)
>>> -             return;
>>> -
>>>       rtnl_lock();
>>> +     if (!np->dev) {
>>> +             rtnl_unlock();
>>> +             return;
>>> +     }
>>>       __netpoll_cleanup(np);
>>> -     rtnl_unlock();
>>> -
>>>       dev_put(np->dev);
>>>       np->dev = NULL;
>>> +     rtnl_unlock();
>>>  }
>>>  EXPORT_SYMBOL(netpoll_cleanup);
>>
>> I know it seems arbitrary, but please do this like:
>>
>>         lock();
>>         if (condition) {
>>                 ...
>>         }
>>         unlock();
>>
>> rather than having multiple return/unlock code paths.
>>
>> This style I am suggesting is easier to audit for locking problems.
> 
> Another alternative is:
> 
> 	lock();
> 
> 	if (!condition)
> 		goto out;
> 
> 	...
> 
> out:
> 	unlock();
> 	return ...;
> }
> 
Yup, this is the alternative I plan to use :-)

Thanks Joe