Re: [PATCH net v2] netlabel: check register_netdevice_notifier() error in netlbl_unlabel_init()

[Jakub Kicinski <kuba@kernel.org>]

AI says that this can never fail in practice. Please make that clear
in the commit message. Take a look at the other comment too, and if
it's incorrect explain in the commit msg why.
---
This is an AI-generated review of your patch.

netlabel: check register_netdevice_notifier() error in netlbl_unlabel_init()

This makes netlbl_unlabel_init() check the return value of
register_netdevice_notifier() instead of ignoring it, and on failure it
clears the netlbl_unlhsh global, waits for a grace period and frees the
hash table before returning the error.

No Link: tags present in the commit header.

> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index 47bae5e48db6c..fe1be424601f6 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c

[ ... ]

> @@ -1419,7 +1420,16 @@ int __init netlbl_unlabel_init(u32 size)
>  	rcu_assign_pointer(netlbl_unlhsh, hsh_tbl);
>  	spin_unlock(&netlbl_unlhsh_lock);
>  
> -	register_netdevice_notifier(&netlbl_unlhsh_netdev_notifier);
> +	err = register_netdevice_notifier(&netlbl_unlhsh_netdev_notifier);
> +	if (err) {

[Severity: Low]
[Finding: main-1-0]
[Sources: opus-5]
Can this branch ever be taken, and can its cleanup ever be observed?

register_netdevice_notifier() in net/core/dev.c only returns the result of
raw_notifier_chain_register() (-EEXIST for an already registered
notifier_block, which cannot happen for the single static
netlbl_unlhsh_netdev_notifier) or an error returned by the per-net
NETDEV_REGISTER replay.  The replay calls
netlbl_unlhsh_netdev_handler(), which always ends with:

net/netlabel/netlabel_unlabeled.c:netlbl_unlhsh_netdev_handler() {
	...
	return NOTIFY_DONE;
}

so the replay cannot fail either.

Even if it did, the sole caller turns the new return value into a
noreturn panic():

net/netlabel/netlabel_kapi.c:netlbl_init() {
	...
	ret_val = netlbl_unlabel_init(NETLBL_UNLHSH_BITSIZE);
	if (ret_val != 0)
		goto init_failure;
	...
init_failure:
	panic("NetLabel: failed to initialize properly (%d)\n", ret_val);
}

which means clearing the global, the synchronize_rcu() wait and both
kfree() calls can never have any observable effect.  The remaining
behaviour change is that a previously ignored condition now reaches
panic() during subsys_initcall, which the changelog does not mention.
Would it be simpler to keep the call unchecked here, or to document the
panic-on-failure behaviour in the changelog?

> +		spin_lock(&netlbl_unlhsh_lock);
> +		rcu_assign_pointer(netlbl_unlhsh, NULL);
> +		spin_unlock(&netlbl_unlhsh_lock);
> +		synchronize_rcu();

[Severity: Low]
[Finding: main-2-2]
[Sources: opus-5]
Is it safe to publish NULL into netlbl_unlhsh here?

Up to now netlbl_unlhsh is assigned exactly once during __init and is
never NULL, and every reader in this file dereferences it without a NULL
check:

net/netlabel/netlabel_unlabeled.c:netlbl_unlhsh_hash() {
	return ifindex & (netlbl_unlhsh_rcu_deref(netlbl_unlhsh)->size - 1);
}

net/netlabel/netlabel_unlabeled.c:netlbl_unlhsh_search_iface() {
	...
	bkt_list = &netlbl_unlhsh_rcu_deref(netlbl_unlhsh)->tbl[bkt];
	...
}

net/netlabel/netlabel_unlabeled.c:netlbl_unlabel_staticlist() {
	...
	for (iter_bkt = skip_bkt;
	     iter_bkt < rcu_dereference(netlbl_unlhsh)->size;
	     iter_bkt++) {
		iter_list = &rcu_dereference(netlbl_unlhsh)->tbl[iter_bkt];
	...
}

netlbl_unlhsh_search_iface() is reached both from the receive path
netlbl_unlabel_getattr() and from netlbl_unlhsh_netdev_handler(), and
the synchronize_rcu() above is a blocking point executed while the
global is already NULL, so the NULL window is not instantaneous.

Nothing oopses today because netlbl_init() panics right away and the
netlabel generic netlink families are only registered later by
netlbl_netlink_init(), so no reader is expected in that window.  If the
panic is ever relaxed into a graceful failure, which is the direction
this patch points at, would every unlabeled lookup and netlink dump then
become a NULL dereference?  Leaving the (empty) table installed on the
failure path would avoid creating the NULL state at all.

> +		kfree(hsh_tbl->tbl);
> +		kfree(hsh_tbl);
> +		return err;
> +	}
>  
>  	return 0;
>  }
-- 
pw-bot: cr