netdev - Re: [patch net-next RFC] net: dsa: move port_setup/teardown to be called outside devlink port registered area

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: <20220726152059.bls6gn7ludfutamy@skbuf>
Date:   Tue, 26 Jul 2022 18:20:59 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        pabeni@...hat.com, edumazet@...gle.com, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com
Subject: Re: [patch net-next RFC] net: dsa: move port_setup/teardown to be
 called outside devlink port registered area

On Tue, Jul 26, 2022 at 04:45:44PM +0200, Jiri Pirko wrote:
> Oh yes, could you please try together with following patch? (nevermind
> chicken-egg problem you may spot now)

Duly ignoring ;)

> Subject: [patch net-next RFC] net: devlink: convert region creation/destroy()
>  to be forbidden on registered devlink/port
> 
> No need to create or destroy region when devlink or devlink ports are
> registered. Limit the possibility to call the region create/destroy()
> only for non-registered devlink or devlink port. Benefit from that and
> avoid need to take devl_lock.
> 
> Signed-off-by: Jiri Pirko <jiri@...dia.com>
> ---
>  drivers/net/netdevsim/dev.c |  8 ++--
>  include/net/devlink.h       |  5 ---
>  net/core/devlink.c          | 78 ++++++++-----------------------------
>  3 files changed, 20 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index 925dc8a5254d..3f0c19e30650 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -557,15 +557,15 @@ static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev,
>  				      struct devlink *devlink)
>  {
>  	nsim_dev->dummy_region =
> -		devl_region_create(devlink, &dummy_region_ops,
> -				   NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX,
> -				   NSIM_DEV_DUMMY_REGION_SIZE);
> +		devlink_region_create(devlink, &dummy_region_ops,
> +				      NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX,
> +				      NSIM_DEV_DUMMY_REGION_SIZE);
>  	return PTR_ERR_OR_ZERO(nsim_dev->dummy_region);
>  }
>  
>  static void nsim_dev_dummy_region_exit(struct nsim_dev *nsim_dev)
>  {
> -	devl_region_destroy(nsim_dev->dummy_region);
> +	devlink_region_destroy(nsim_dev->dummy_region);
>  }
>  
>  static int
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 9edb4a28cf30..2416750e050d 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1666,10 +1666,6 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
>  int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
>  				       union devlink_param_value init_val);
>  void devlink_param_value_changed(struct devlink *devlink, u32 param_id);
> -struct devlink_region *devl_region_create(struct devlink *devlink,
> -					  const struct devlink_region_ops *ops,
> -					  u32 region_max_snapshots,
> -					  u64 region_size);
>  struct devlink_region *
>  devlink_region_create(struct devlink *devlink,
>  		      const struct devlink_region_ops *ops,
> @@ -1678,7 +1674,6 @@ struct devlink_region *
>  devlink_port_region_create(struct devlink_port *port,
>  			   const struct devlink_port_region_ops *ops,
>  			   u32 region_max_snapshots, u64 region_size);
> -void devl_region_destroy(struct devlink_region *region);
>  void devlink_region_destroy(struct devlink_region *region);
>  void devlink_port_region_destroy(struct devlink_region *region);
>  
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 4e0c4f9265e8..15d28aba69fc 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -5701,8 +5701,7 @@ static void devlink_nl_region_notify(struct devlink_region *region,
>  	struct sk_buff *msg;
>  
>  	WARN_ON(cmd != DEVLINK_CMD_REGION_NEW && cmd != DEVLINK_CMD_REGION_DEL);
> -	if (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
> -		return;
> +	ASSERT_DEVLINK_REGISTERED(devlink);
>  
>  	msg = devlink_nl_region_notify_build(region, snapshot, cmd, 0, 0);
>  	if (IS_ERR(msg))
> @@ -11131,21 +11130,22 @@ void devlink_param_value_changed(struct devlink *devlink, u32 param_id)
>  EXPORT_SYMBOL_GPL(devlink_param_value_changed);
>  
>  /**
> - * devl_region_create - create a new address region
> + * devlink_region_create - create a new address region
>   *
>   * @devlink: devlink
>   * @ops: region operations and name
>   * @region_max_snapshots: Maximum supported number of snapshots for region
>   * @region_size: size of region
>   */
> -struct devlink_region *devl_region_create(struct devlink *devlink,
> -					  const struct devlink_region_ops *ops,
> -					  u32 region_max_snapshots,
> -					  u64 region_size)
> +struct devlink_region *
> +devlink_region_create(struct devlink *devlink,
> +		      const struct devlink_region_ops *ops,
> +		      u32 region_max_snapshots,
> +		      u64 region_size)
>  {
>  	struct devlink_region *region;
>  
> -	devl_assert_locked(devlink);
> +	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
>  
>  	if (WARN_ON(!ops) || WARN_ON(!ops->destructor))
>  		return ERR_PTR(-EINVAL);
> @@ -11164,35 +11164,9 @@ struct devlink_region *devl_region_create(struct devlink *devlink,
>  	INIT_LIST_HEAD(&region->snapshot_list);
>  	mutex_init(&region->snapshot_lock);
>  	list_add_tail(&region->list, &devlink->region_list);
> -	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);
>  
>  	return region;
>  }
> -EXPORT_SYMBOL_GPL(devl_region_create);
> -
> -/**
> - *	devlink_region_create - create a new address region
> - *
> - *	@devlink: devlink
> - *	@ops: region operations and name
> - *	@region_max_snapshots: Maximum supported number of snapshots for region
> - *	@region_size: size of region
> - *
> - *	Context: Takes and release devlink->lock <mutex>.
> - */
> -struct devlink_region *
> -devlink_region_create(struct devlink *devlink,
> -		      const struct devlink_region_ops *ops,
> -		      u32 region_max_snapshots, u64 region_size)
> -{
> -	struct devlink_region *region;
> -
> -	devl_lock(devlink);
> -	region = devl_region_create(devlink, ops, region_max_snapshots,
> -				    region_size);
> -	devl_unlock(devlink);
> -	return region;
> -}
>  EXPORT_SYMBOL_GPL(devlink_region_create);
>  
>  /**

Were you on net-next when you generated this patch? Here's what I have
at 11164:

devlink_port_region_create:
	if (devlink_port_region_get_by_name(port, ops->name)) {
		err = -EEXIST;
		goto unlock;
	}

	region = kzalloc(sizeof(*region), GFP_KERNEL);
	if (!region) {

My end of devl_region_create() and start of devlink_region_create() are
at 11106, but notice how I lack the mutex_init(&region->snapshot_lock)
that you have in your context:

	INIT_LIST_HEAD(&region->snapshot_list);
	list_add_tail(&region->list, &devlink->region_list);
	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);

	return region;
}
EXPORT_SYMBOL_GPL(devl_region_create);

Would you mind regenerating this patch rather than letting me bodge it?