netdev - Re: [PATCH 06/10] devlink: convert snapshot id getter to return an error

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: <20200325110425.6fdf6cb3@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 25 Mar 2020 11:04:25 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH 06/10] devlink: convert snapshot id getter to return an
 error

On Tue, 24 Mar 2020 15:34:41 -0700 Jacob Keller wrote:
> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index f7621ccb7b88..f9420b77e5fd 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -45,8 +45,7 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
>  {
>  	struct nsim_dev *nsim_dev = file->private_data;
>  	void *dummy_data;
> -	int err;
> -	u32 id;
> +	int err, id;
>  
>  	dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
>  	if (!dummy_data)
> @@ -55,6 +54,10 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
>  	get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
>  
>  	id = devlink_region_snapshot_id_get(priv_to_devlink(nsim_dev));
> +	if (id < 0) {
> +		pr_err("Failed to get snapshot id\n");
> +		return id;
> +	}
>  	err = devlink_region_snapshot_create(nsim_dev->dummy_region,
>  					     dummy_data, id);
>  	if (err) {

Hmm... next patch introduces some ref counting on the ID AFAICT,
should there be some form of snapshot_id_put(), once the driver is 
done creating the regions it wants?

First what if driver wants to create two snapshots with the same ID but
user space manages to delete the first one before second one is created.

Second what if create fails, won't the snapshot ID just stay in XA with
count of 0 forever?