[PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Jeremy Kerr]

The current kunit infrastructure defines its own module_init() when
built as a module, which conflicts with the mctp core's own.

So, only allow MCTP_TEST when both MCTP and KUNIT are built-in.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
 net/mctp/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mctp/Kconfig b/net/mctp/Kconfig
index 15267a5043d9..868c92272cbd 100644
--- a/net/mctp/Kconfig
+++ b/net/mctp/Kconfig
@@ -13,6 +13,6 @@ menuconfig MCTP
 	  channel.
 
 config MCTP_TEST
-        tristate "MCTP core tests" if !KUNIT_ALL_TESTS
-        depends on MCTP && KUNIT
+        bool "MCTP core tests" if !KUNIT_ALL_TESTS
+        depends on MCTP=y && KUNIT=y
         default KUNIT_ALL_TESTS
-- 
2.30.2

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

[Jeremy Kerr]

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

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

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[David Gow]

On Sat, Oct 2, 2021 at 10:27 AM Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> The current kunit infrastructure defines its own module_init() when
> built as a module, which conflicts with the mctp core's own.
>
> So, only allow MCTP_TEST when both MCTP and KUNIT are built-in.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---

This looks good to me. I don't think you'll be the only person to hit
this issue, so -- while it's probably overall nicer if tests can sit
in their own module -- we'll look into finding a way of supporting
this with KUnit at some point. In the meantime, though, this is a
reasonable workaround.

Reviewed-by: David Gow <davidgow@google.com>

-- David

>  net/mctp/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/mctp/Kconfig b/net/mctp/Kconfig
> index 15267a5043d9..868c92272cbd 100644
> --- a/net/mctp/Kconfig
> +++ b/net/mctp/Kconfig
> @@ -13,6 +13,6 @@ menuconfig MCTP
>           channel.
>
>  config MCTP_TEST
> -        tristate "MCTP core tests" if !KUNIT_ALL_TESTS
> -        depends on MCTP && KUNIT
> +        bool "MCTP core tests" if !KUNIT_ALL_TESTS
> +        depends on MCTP=y && KUNIT=y
>          default KUNIT_ALL_TESTS
> --
> 2.30.2
>

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

[David Gow]

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
>

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[David Miller]

From: Jeremy Kerr <jk@codeconstruct.com.au>
Date: Sat,  2 Oct 2021 10:26:55 +0800

> The current kunit infrastructure defines its own module_init() when
> built as a module, which conflicts with the mctp core's own.
> 
> So, only allow MCTP_TEST when both MCTP and KUNIT are built-in.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

Jeremy I had to revert your entire series because of this.

You will need rseubmit the entire series with this build failure fixed.

Thasnk you.

Re: [PATCH net-next 1/2] mctp: test: disallow MCTP_TEST when building as a module

[Jeremy Kerr]

Hi David,

> Jeremy I had to revert your entire series because of this.
> 
> You will need rseubmit the entire series with this build failure
> fixed.

OK, thanks for letting me know, apologies for the breakage. Looks like
my MCTP=m pre-send check didn't end up enabling MCTP_TEST...

v2 coming shortly.

Cheers,


Jeremy

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

[Jeremy Kerr]

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