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: <c119488b-1b27-7c65-7e60-a64bdda585c1@intel.com>
Date:   Wed, 25 Mar 2020 18:33:41 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
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 3/25/2020 11:04 AM, Jakub Kicinski wrote:
> 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?
> 

Ah, yep good catch. I'll add a _put* function and make drivers call this.

Thanks,
Jake