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: <20200325191356.GF11304@nanopsycho.orion>
Date:   Wed, 25 Mar 2020 20:13:56 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Jacob Keller <jacob.e.keller@...el.com>, netdev@...r.kernel.org,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH 06/10] devlink: convert snapshot id getter to return an
 error

Wed, Mar 25, 2020 at 07:04:25PM CET, kuba@...nel.org 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?

Yeah, that seems like a race condition this is introducing.