diff mbox series

[net-next,2/2] mctp: test: defer mdev setup until we've registered

Message ID 20211002022656.1681956-2-jk@codeconstruct.com.au (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series [net-next,1/2] mctp: test: disallow MCTP_TEST when building as a module | expand

Checks

Context Check Description
netdev/apply fail Patch does not apply to net-next
netdev/tree_selection success Clearly marked for net-next

Commit Message

Jeremy Kerr Oct. 2, 2021, 2:26 a.m. UTC
The MCTP device isn't available until we've registered the netdev, so
defer storing our convenience pointer.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
 net/mctp/test/utils.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

David Gow Oct. 2, 2021, 3:16 a.m. UTC | #1
On Sat, Oct 2, 2021 at 10:27 AM Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> The MCTP device isn't available until we've registered the netdev, so
> defer storing our convenience pointer.
>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---

Haha -- you sent this just as I'd come up with the same patch here. :-)

With these changes, alongside the rt->dev == NULL in
mctp_route_release() crash fix mentioned in [1], the tests all pass on
my system. (They also pass under KASAN, which bodes well.)

This fix is,
Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

[1]: https://lore.kernel.org/linux-kselftest/163309440949.24017.15314464452259318665.git-patchwork-notify@kernel.org/T/#m1a319b6932dd2bffaf78ab2d3b4c399282f7bda2



>  net/mctp/test/utils.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/net/mctp/test/utils.c b/net/mctp/test/utils.c
> index e2ab1f3da357..cc6b8803aa9d 100644
> --- a/net/mctp/test/utils.c
> +++ b/net/mctp/test/utils.c
> @@ -46,17 +46,17 @@ struct mctp_test_dev *mctp_test_create_dev(void)
>         dev = netdev_priv(ndev);
>         dev->ndev = ndev;
>
> -       rcu_read_lock();
> -       dev->mdev = __mctp_dev_get(ndev);
> -       mctp_dev_hold(dev->mdev);
> -       rcu_read_unlock();
> -
>         rc = register_netdev(ndev);
>         if (rc) {
>                 free_netdev(ndev);
>                 return NULL;
>         }
>
> +       rcu_read_lock();
> +       dev->mdev = __mctp_dev_get(ndev);
> +       mctp_dev_hold(dev->mdev);
> +       rcu_read_unlock();
> +
>         return dev;
>  }
>
> --
> 2.30.2
>
Jeremy Kerr Oct. 3, 2021, 3:24 a.m. UTC | #2
Hi David,

> Haha -- you sent this just as I'd come up with the same patch here.
> :-)
> 
> With these changes, alongside the rt->dev == NULL in
> mctp_route_release() crash fix mentioned in [1], the tests all pass
> on
> my system. (They also pass under KASAN, which bodes well.)

Awesome, thanks for checking these out. I've since sent a v2 with the
fixes integrated, in order to not break davem's build.

I've refined the rt->dev == NULL case a little; rather than allowing
->dev == NULL in the core code (which should never happen), I've
modified the test's route refcounting so that the route destroy path
should only ever hit the test's own destructor instead (which allows
!rt->dev cases). This means we can keep the ->dev != NULL assumption in
the core, and still handle tests where our fake route->dev is unset.

Cheers,


Jeremy
diff mbox series

Patch

diff --git a/net/mctp/test/utils.c b/net/mctp/test/utils.c
index e2ab1f3da357..cc6b8803aa9d 100644
--- a/net/mctp/test/utils.c
+++ b/net/mctp/test/utils.c
@@ -46,17 +46,17 @@  struct mctp_test_dev *mctp_test_create_dev(void)
 	dev = netdev_priv(ndev);
 	dev->ndev = ndev;
 
-	rcu_read_lock();
-	dev->mdev = __mctp_dev_get(ndev);
-	mctp_dev_hold(dev->mdev);
-	rcu_read_unlock();
-
 	rc = register_netdev(ndev);
 	if (rc) {
 		free_netdev(ndev);
 		return NULL;
 	}
 
+	rcu_read_lock();
+	dev->mdev = __mctp_dev_get(ndev);
+	mctp_dev_hold(dev->mdev);
+	rcu_read_unlock();
+
 	return dev;
 }