netdev - Re: [PATCH net 1/3] ionic: Fix netdev notifier unregister on failure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <564b9d98-4d64-40ab-a523-4487712430dd@intel.com>
Date: Tue, 10 Dec 2024 12:59:31 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Shannon Nelson <shannon.nelson@....com>, <netdev@...r.kernel.org>,
	<davem@...emloft.net>, <kuba@...nel.org>, <edumazet@...gle.com>,
	<pabeni@...hat.com>, <andrew+netdev@...n.ch>
CC: <brett.creeley@....com>
Subject: Re: [PATCH net 1/3] ionic: Fix netdev notifier unregister on failure



On 12/10/2024 9:48 AM, Shannon Nelson wrote:
> From: Brett Creeley <brett.creeley@....com>
> 
> If register_netdev() fails, then the driver leaks the netdev notifier.
> Fix this by calling ionic_lif_unregister() on register_netdev()
> failure. This will also call ionic_lif_unregister_phc() if it has
> already been registered.
> 
> While at it, remove the empty and unused nb_work and associated
> ionic_lif_notify_work() function.
> 
> Fixes: 30b87ab4c0b3 ("ionic: remove lif list concept")
> Signed-off-by: Brett Creeley <brett.creeley@....com>
> Signed-off-by: Shannon Nelson <shannon.nelson@....com>
> ---

I'm not certain about the inclusion of cleanup to drop unused code in
the same commit as an obvious fix. However, the changes as a whole seem
ok to me:

With or without splitting:

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>


>  drivers/net/ethernet/pensando/ionic/ionic.h     |  1 -
>  drivers/net/ethernet/pensando/ionic/ionic_lif.c | 11 ++---------
>  2 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
> index 1c61390677f7..faaf96af506d 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic.h
> @@ -59,7 +59,6 @@ struct ionic {
>  	DECLARE_BITMAP(intrs, IONIC_INTR_CTRL_REGS_MAX);
>  	cpumask_var_t *affinity_masks;
>  	struct delayed_work doorbell_check_dwork;
> -	struct work_struct nb_work;
>  	struct notifier_block nb;
>  	struct rw_semaphore vf_op_lock;	/* lock for VF operations */
>  	struct ionic_vf *vfs;
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index 40496587b2b3..bfa24c659d84 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -3804,10 +3804,6 @@ int ionic_lif_init(struct ionic_lif *lif)
>  	return err;
>  }
>  
> -static void ionic_lif_notify_work(struct work_struct *ws)
> -{
> -}
> -
>  static void ionic_lif_set_netdev_info(struct ionic_lif *lif)
>  {
>  	struct ionic_admin_ctx ctx = {
> @@ -3858,8 +3854,6 @@ int ionic_lif_register(struct ionic_lif *lif)
>  
>  	ionic_lif_register_phc(lif);
>  
> -	INIT_WORK(&lif->ionic->nb_work, ionic_lif_notify_work);
> -
>  	lif->ionic->nb.notifier_call = ionic_lif_notify;
>  
>  	err = register_netdevice_notifier(&lif->ionic->nb);
> @@ -3869,8 +3863,8 @@ int ionic_lif_register(struct ionic_lif *lif)
>  	/* only register LIF0 for now */
>  	err = register_netdev(lif->netdev);
>  	if (err) {
> -		dev_err(lif->ionic->dev, "Cannot register net device, aborting\n");
> -		ionic_lif_unregister_phc(lif);
> +		dev_err(lif->ionic->dev, "Cannot register net device: %d, aborting\n", err);
> +		ionic_lif_unregister(lif);
>  		return err;
>  	}
>  
> @@ -3885,7 +3879,6 @@ void ionic_lif_unregister(struct ionic_lif *lif)
>  {
>  	if (lif->ionic->nb.notifier_call) {
>  		unregister_netdevice_notifier(&lif->ionic->nb);
> -		cancel_work_sync(&lif->ionic->nb_work);
>  		lif->ionic->nb.notifier_call = NULL;
>  	}
>