netdev - Re: [PATCH net-next v7 4/9] devlink: Allow for devlink-rate nodes parent reassignment

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]

Date:   Fri, 28 Oct 2022 11:35:58 +0200
From:   "Wilczynski, Michal" <michal.wilczynski@...el.com>
To:     Przemek Kitszel <przemyslaw.kitszel@...el.com>,
        <netdev@...r.kernel.org>
CC:     <alexandr.lobakin@...el.com>, <jacob.e.keller@...el.com>,
        <jesse.brandeburg@...el.com>, <anthony.l.nguyen@...el.com>,
        <kuba@...nel.org>, <ecree.xilinx@...il.com>, <jiri@...nulli.us>
Subject: Re: [PATCH net-next v7 4/9] devlink: Allow for devlink-rate nodes
 parent reassignment



On 10/28/2022 12:34 AM, Przemek Kitszel wrote:
> From:   Michal Wilczynski <michal.wilczynski@...el.com>
> Date:   Thu, 27 Oct 2022 15:00:44 +0200
>
> [...]
>
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 864fa0967b7a..1e0c1b0376bf 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -1875,10 +1875,9 @@ devlink_nl_rate_parent_node_set(struct devlink_rate *devlink_rate,
>>   	int err = -EOPNOTSUPP;
>>   
>>   	parent = devlink_rate->parent;
>> -	if (parent && len) {
>> -		NL_SET_ERR_MSG_MOD(info->extack, "Rate object already has parent.");
>> -		return -EBUSY;
>> -	} else if (parent && !len) {
>> +
>> +	/* if a parent is already set, just reassign the parent */
>> +	if (parent && !len) {
> Comment that you have added should be placed way below, here it is misleading.

In my mind it made sense, cause we're handling an additional case here that
wasn't handled before. I think I'll just remove the comment then if you
find it misleading.

>
>>   		if (devlink_rate_is_leaf(devlink_rate))
>>   			err = ops->rate_leaf_parent_set(devlink_rate, NULL,
>>   							devlink_rate->priv, NULL,
>> @@ -1892,7 +1891,7 @@ devlink_nl_rate_parent_node_set(struct devlink_rate *devlink_rate,
>>   
>>   		refcount_dec(&parent->refcnt);
>>   		devlink_rate->parent = NULL;
>> -	} else if (!parent && len) {
>> +	} else if (len) {
>>   		parent = devlink_rate_node_get_by_name(devlink, parent_name);
>>   		if (IS_ERR(parent))
>>   			return -ENODEV;
>> @@ -1919,6 +1918,10 @@ devlink_nl_rate_parent_node_set(struct devlink_rate *devlink_rate,
>>   		if (err)
>>   			return err;
>>   
> Comment above makes more sense here, likely combined with the one just below.

They would duplicate in my mind, so I'll just remove the upper one.

>
>> +		if (devlink_rate->parent)
>> +			/* we're reassigning to other parent in this case */
>> +			refcount_dec(&devlink_rate->parent->refcnt);
>> +
>>   		refcount_inc(&parent->refcnt);
>>   		devlink_rate->parent = parent;
>>   	}
>> --
> Thanks for splitting this patch out of the other, change itself is easier
> to follow now. Code (modulo comments) is correct, you could add my Reviewed-by
> after comment fix.
>
> Side note: ops (rate_{leaf|node}_parent_set) lack documentation. There is also
> not much usage of them as of now, so maybe we could extend them to actually do
> refcount_inc + refcount_dec (if applicable) + set pointers.

I'm not sure I follow you there, those are function pointers, and are 
supposed
to be implemented in the driver.

> OTOH: As of now those are more of "on-event" callbacks, not "do-something",
> what is further confirmed in name (word "set" on the end, not begining).
>
> --PK