[PATCH net] mctp: Remove device type check at unregister

[PATCH net] mctp: Remove device type check at unregister

[Matt Johnston]

The check could be incorrectly triggered if a netdev
changes its type after register. That is possible for a tun device
using TUNSETLINK ioctl, resulting in mctp unregister failing
and the netdev unregister waiting forever.

This was encountered by https://github.com/openthread/openthread/issues/8523

The check is not required, it was added in an attempt to track down mctp_ptr
being set unexpectedly, which should not happen in normal operation.

Fixes: 7b1871af75f3 ("mctp: Warn if pointer is set for a wrong dev type")
Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---
 net/mctp/device.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/mctp/device.c b/net/mctp/device.c
index 99a3bda8852f..dec730b5fe7e 100644
--- a/net/mctp/device.c
+++ b/net/mctp/device.c
@@ -429,12 +429,6 @@ static void mctp_unregister(struct net_device *dev)
 	struct mctp_dev *mdev;
 
 	mdev = mctp_dev_get_rtnl(dev);
-	if (mdev && !mctp_known(dev)) {
-		// Sanity check, should match what was set in mctp_register
-		netdev_warn(dev, "%s: BUG mctp_ptr set for unknown type %d",
-			    __func__, dev->type);
-		return;
-	}
 	if (!mdev)
 		return;
 
-- 
2.37.2

Re: [PATCH net] mctp: Remove device type check at unregister

[Alexander H Duyck]

On Wed, 2022-12-14 at 14:10 +0800, Matt Johnston wrote:
> The check could be incorrectly triggered if a netdev
> changes its type after register. That is possible for a tun device
> using TUNSETLINK ioctl, resulting in mctp unregister failing
> and the netdev unregister waiting forever.
> 
> This was encountered by https://github.com/openthread/openthread/issues/8523
> 
> The check is not required, it was added in an attempt to track down mctp_ptr
> being set unexpectedly, which should not happen in normal operation.
> 
> Fixes: 7b1871af75f3 ("mctp: Warn if pointer is set for a wrong dev type")
> Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
> ---
>  net/mctp/device.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/net/mctp/device.c b/net/mctp/device.c
> index 99a3bda8852f..dec730b5fe7e 100644
> --- a/net/mctp/device.c
> +++ b/net/mctp/device.c
> @@ -429,12 +429,6 @@ static void mctp_unregister(struct net_device *dev)
>  	struct mctp_dev *mdev;
>  
>  	mdev = mctp_dev_get_rtnl(dev);
> -	if (mdev && !mctp_known(dev)) {
> -		// Sanity check, should match what was set in mctp_register
> -		netdev_warn(dev, "%s: BUG mctp_ptr set for unknown type %d",
> -			    __func__, dev->type);
> -		return;
> -	}
>  	if (!mdev)
>  		return;
>  

It looks like this is incomplete if we are going to allow for these
type of changes. We might as well also remove the block in
mctp_register that was doing a similar check for devices that already
have the mctp_ptr set. Otherwise you will likely need to follow up on
this later.

Alternatively if you are wanting to be consistent you could just drop
the "return", in which case you will still get the warn in both cases
but it doesn't seem to affect behavior otherwise.

In addition you may want to look at registering a notifier to catch
NETDEV_PRE_TYPE_CHANGE and NETDEV_PRE_TYPE_CHANGE. It would enable you
to catch these types of things in the future if you are wanting to
allow changing types.

Re: [PATCH net] mctp: Remove device type check at unregister

[Matt Johnston]

On Wed, 2022-12-14 at 08:42 -0800, Alexander H Duyck wrote:
> > diff --git a/net/mctp/device.c b/net/mctp/device.c
> > index 99a3bda8852f..dec730b5fe7e 100644
> > --- a/net/mctp/device.c
> > +++ b/net/mctp/device.c
> > @@ -429,12 +429,6 @@ static void mctp_unregister(struct net_device *dev)
> >  	struct mctp_dev *mdev;
> >  
> >  	mdev = mctp_dev_get_rtnl(dev);
> > -	if (mdev && !mctp_known(dev)) {
> > -		// Sanity check, should match what was set in mctp_register
> > -		netdev_warn(dev, "%s: BUG mctp_ptr set for unknown type %d",
> > -			    __func__, dev->type);
> > -		return;
> > -	}
> >  	if (!mdev)
> >  		return;
> >  
> 
> It looks like this is incomplete if we are going to allow for these
> type of changes. We might as well also remove the block in
> mctp_register that was doing a similar check for devices that already
> have the mctp_ptr set. Otherwise you will likely need to follow up on
> this later.

I'm not sure it's possible to hit that, I'll remove it in a v2.

Thanks for the review.

Cheers,
Matt