register_netdevice_notifier() may fail, but macvlan_init_module() does
not handle the failure.  Handle the failure by returning an error.

Fixes: b863ceb7ddce ("[NET]: Add macvlan driver")
Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
 drivers/net/macvlan.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 24298a33e0e9..ae2f1a8325a5 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1849,7 +1849,9 @@ static int __init macvlan_init_module(void)
 {
 	int err;
 
-	register_netdevice_notifier(&macvlan_notifier_block);
+	err = register_netdevice_notifier(&macvlan_notifier_block);
+	if (err < 0)
+		return err;
 
 	err = macvlan_link_register(&macvlan_link_ops);
 	if (err < 0)
-- 
2.45.2

On Wed, Jul 24, 2024 at 3:56 PM Shigeru Yoshida <syoshida@redhat.com> wrote:
>
> register_netdevice_notifier() may fail, but macvlan_init_module() does
> not handle the failure.  Handle the failure by returning an error.

How could this fail exactly ? Please provide details, because I do not
think it can.

>
> Fixes: b863ceb7ddce ("[NET]: Add macvlan driver")
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> ---
>  drivers/net/macvlan.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 24298a33e0e9..ae2f1a8325a5 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -1849,7 +1849,9 @@ static int __init macvlan_init_module(void)
>  {
>         int err;
>
> -       register_netdevice_notifier(&macvlan_notifier_block);
> +       err = register_netdevice_notifier(&macvlan_notifier_block);
> +       if (err < 0)
> +               return err;
>
>         err = macvlan_link_register(&macvlan_link_ops);
>         if (err < 0)
> --
> 2.45.2
>

On 7/25/24 11:44, Eric Dumazet wrote:
> On Wed, Jul 24, 2024 at 3:56 PM Shigeru Yoshida <syoshida@redhat.com> wrote:
>>
>> register_netdevice_notifier() may fail, but macvlan_init_module() does
>> not handle the failure.  Handle the failure by returning an error.
> 
> How could this fail exactly ? Please provide details, because I do not
> think it can.

Yup, it looks like the registration can't fail for macvlan.

It's better to avoid adding unneeded checks, to reduce noise on the 
tree, keep stable backport easy, etc.

Thanks,

Paolo

On Thu, Jul 25, 2024 at 12:13 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
>
>
> On 7/25/24 11:44, Eric Dumazet wrote:
> > On Wed, Jul 24, 2024 at 3:56 PM Shigeru Yoshida <syoshida@redhat.com> wrote:
> >>
> >> register_netdevice_notifier() may fail, but macvlan_init_module() does
> >> not handle the failure.  Handle the failure by returning an error.
> >
> > How could this fail exactly ? Please provide details, because I do not
> > think it can.
>
> Yup, it looks like the registration can't fail for macvlan.
>
> It's better to avoid adding unneeded checks, to reduce noise on the
> tree, keep stable backport easy, etc.

Shigeru, you could send a debug patch when net-next reopens next week,
so that we do not get another attempt
on fixing a non-existent bug.

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 24298a33e0e94851ebf9c704c723f25ac7bf5eec..0803fcf8df4c56ede10597c862288c7aa887160e
100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1849,7 +1849,8 @@ static int __init macvlan_init_module(void)
 {
        int err;

-       register_netdevice_notifier(&macvlan_notifier_block);
+       err = register_netdevice_notifier(&macvlan_notifier_block);
+       DEBUG_NET_WARN_ON_ONCE(err < 0);

        err = macvlan_link_register(&macvlan_link_ops);
        if (err < 0)

Hi Eric, Paolo,

On Thu, 25 Jul 2024 13:53:27 +0200, Eric Dumazet wrote:
> On Thu, Jul 25, 2024 at 12:13 PM Paolo Abeni <pabeni@redhat.com> wrote:
>>
>>
>>
>> On 7/25/24 11:44, Eric Dumazet wrote:
>> > On Wed, Jul 24, 2024 at 3:56 PM Shigeru Yoshida <syoshida@redhat.com> wrote:
>> >>
>> >> register_netdevice_notifier() may fail, but macvlan_init_module() does
>> >> not handle the failure.  Handle the failure by returning an error.
>> >
>> > How could this fail exactly ? Please provide details, because I do not
>> > think it can.
>>
>> Yup, it looks like the registration can't fail for macvlan.
>>
>> It's better to avoid adding unneeded checks, to reduce noise on the
>> tree, keep stable backport easy, etc.

Thank you for your comments, and it's my bad, sorry.

I happened to look at macvlan's code and found this.  I compared this
with others like macvtap and I assumed that registering for notifier
needs error handling without checking carefully.

> Shigeru, you could send a debug patch when net-next reopens next week,
> so that we do not get another attempt
> on fixing a non-existent bug.

I think we don't need this DEBUG_NET_WARN_ON_ONCE() because it makes
the source code a little messy.

Next time I find something, I'll be more careful.

Thanks,
Shigeru

> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 24298a33e0e94851ebf9c704c723f25ac7bf5eec..0803fcf8df4c56ede10597c862288c7aa887160e
> 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -1849,7 +1849,8 @@ static int __init macvlan_init_module(void)
>  {
>         int err;
> 
> -       register_netdevice_notifier(&macvlan_notifier_block);
> +       err = register_netdevice_notifier(&macvlan_notifier_block);
> +       DEBUG_NET_WARN_ON_ONCE(err < 0);
> 
>         err = macvlan_link_register(&macvlan_link_ops);
>         if (err < 0)
> 

On Wed, Jul 24, 2024 at 10:56:22PM +0900, Shigeru Yoshida wrote:
> register_netdevice_notifier() may fail, but macvlan_init_module() does
> not handle the failure.  Handle the failure by returning an error.
> 
> Fixes: b863ceb7ddce ("[NET]: Add macvlan driver")
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>

Reviewed-by: Simon Horman <horms@kernel.org>