| Message ID | 20221214061044.892446-1-matt@codeconstruct.com.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] mctp: Remove device type check at unregister | expand |
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.
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
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;
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(-)