netdev - RE: [PATCH net-next 01/10] netdevsim: fib: Convert the current occupancy to an atomic variable

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: <DM6PR12MB30665BEF4DBA4B1BA697E23ACBBB9@DM6PR12MB3066.namprd12.prod.outlook.com>
Date:   Wed, 27 Jan 2021 10:51:17 +0000
From:   Amit Cohen <amcohen@...dia.com>
To:     David Ahern <dsahern@...il.com>, Ido Schimmel <idosch@...sch.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        Roopa Prabhu <roopa@...dia.com>,
        Donald Sharp <sharpd@...dia.com>,
        Benjamin Poirier <bpoirier@...dia.com>,
        mlxsw <mlxsw@...dia.com>, Ido Schimmel <idosch@...dia.com>
Subject: RE: [PATCH net-next 01/10] netdevsim: fib: Convert the current
 occupancy to an atomic variable



>-----Original Message-----
>From: David Ahern <dsahern@...il.com>
>Sent: Wednesday, January 27, 2021 6:33
>To: Ido Schimmel <idosch@...sch.org>; netdev@...r.kernel.org
>Cc: davem@...emloft.net; kuba@...nel.org; Amit Cohen <amcohen@...dia.com>; Roopa Prabhu <roopa@...dia.com>; Donald
>Sharp <sharpd@...dia.com>; Benjamin Poirier <bpoirier@...dia.com>; mlxsw <mlxsw@...dia.com>; Ido Schimmel
><idosch@...dia.com>
>Subject: Re: [PATCH net-next 01/10] netdevsim: fib: Convert the current occupancy to an atomic variable
>
>On 1/26/21 6:23 AM, Ido Schimmel wrote:
>> @@ -889,22 +882,29 @@ static void nsim_nexthop_destroy(struct
>> nsim_nexthop *nexthop)  static int nsim_nexthop_account(struct nsim_fib_data *data, u64 occ,
>>  				bool add, struct netlink_ext_ack *extack)  {
>> -	int err = 0;
>> +	int i, err = 0;
>>
>>  	if (add) {
>> -		if (data->nexthops.num + occ <= data->nexthops.max) {
>> -			data->nexthops.num += occ;
>> -		} else {
>> -			err = -ENOSPC;
>> -			NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported nexthops");
>> -		}
>> +		for (i = 0; i < occ; i++)
>> +			if (!atomic64_add_unless(&data->nexthops.num, 1,
>> +						 data->nexthops.max)) {
>
>seems like this can be
>		if (!atomic64_add_unless(&data->nexthops.num, occ,
>					 data->nexthops.max)) {

atomic64_add_unless(x, y, z) adds y to x if x was not already z.
Which means that when for example num=2, occ=2, max=3:
atomic64_add_unless(&data->nexthops.num, occ, data->nexthops.max) won't fail when it should.

This situation is realistic and actually with atomic64_add_unless() selftests/drivers/net/netdevsim/nexthop.sh fails.

>
>and then the err_num_decrease is not needed
>
>> +				err = -ENOSPC;
>> +				NL_SET_ERR_MSG_MOD(extack, "Exceeded number of supported nexthops");
>> +				goto err_num_decrease;
>> +			}
>>  	} else {
>> -		if (WARN_ON(occ > data->nexthops.num))
>> +		if (WARN_ON(occ > atomic64_read(&data->nexthops.num)))
>>  			return -EINVAL;
>> -		data->nexthops.num -= occ;
>> +		atomic64_sub(occ, &data->nexthops.num);
>>  	}
>>
>>  	return err;
>> +
>> +err_num_decrease:
>> +	for (i--; i >= 0; i--)
>> +		atomic64_dec(&data->nexthops.num);
>
>and if this path is really needed, why not atomic64_sub here?
>
>> +	return err;
>> +
>>  }
>>
>>  static int nsim_nexthop_add(struct nsim_fib_data *data,
>>