netdev - Re: [PATCH] devlink: track snapshot id usage count using an xarray

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] [day] [month] [year] [list]

Message-ID: <e0b46a11-4174-163b-401b-bdfe1d6e4f5c@intel.com>
Date:   Wed, 25 Mar 2020 18:43:21 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH] devlink: track snapshot id usage count using an xarray



On 3/25/2020 9:08 AM, Jiri Pirko wrote:
> Tue, Mar 24, 2020 at 11:34:42PM CET, jacob.e.keller@...el.com wrote:
>> +static int __devlink_snapshot_id_increment(struct devlink *devlink, u32 id)
>> +{
>> +	unsigned long count;
>> +	int err;
>> +	void *p;
>> +
>> +	lockdep_assert_held(&devlink->lock);
>> +
>> +	p = xa_load(&devlink->snapshot_ids, id);
>> +	if (!p)
>> +		return -EEXIST;
> 
> This is confusing. You should return rather -ENOTEXIST, if it existed :)
> -EINVAL and WARN_ON. This should never happen
> 

Yea this is confusing. I'll add a WARN_ON, and use EINVAL.

> 
>> +
>> +	if (!xa_is_value(p))
>> +		return -EINVAL;
>> +
>> +	count = xa_to_value(p);
>> +	count++;
>> +
>> +	err = xa_err(xa_store(&devlink->snapshot_ids, id, xa_mk_value(count),
>> +			      GFP_KERNEL));
> 
> Just return here and remove err variable.

Yep.

>> -	if (devlink->snapshot_id >= INT_MAX)
>> -		return -ENOSPC;
>> +	/* xa_limit_31b ensures the id will be between 0 and INT_MAX */
> 
> Well, currently the snapshot_id is u32. Even the netlink attr is u32.
> I believe we should not limit it here.
> 
> Please have this as xa_limit_32b.
> 

Currently we can't do that. Negative values are used to represent
errors, and allowing up to u32 would break the get function, because
large IDs would be interpreted as errors.

I'll clean this up in a patch first before the xarray stuff.

Thanks,
Jake