netdev - Re: [PATCH net] net/sched: cls_api: Fix flow action initialization

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:   Tue, 19 Jul 2022 12:32:59 +0300
From:   Oz Shlomo <ozsh@...dia.com>
To:     Baowen Zheng <baowen.zheng@...igine.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     Simon Horman <simon.horman@...igine.com>,
        Roi Dayan <roid@...dia.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH net] net/sched: cls_api: Fix flow action initialization

Hi Baowen,

On 7/19/2022 4:30 AM, Baowen Zheng wrote:
> Hi Oz:
> On July 18, 2022 8:29 PM, Oz Shlomo wrote:
>> On 7/18/2022 4:40 AM, Baowen Zheng wrote:
>>> On Sunday, July 17, 2022 4:26 PM, Oz Shlomo wrote:
>>>> Subject: [PATCH net] net/sched: cls_api: Fix flow action
>>>> initialization
>>>>
>>>> The cited commit refactored the flow action initialization sequence
>>>> to use an interface method when translating tc action instances to flow
>> offload objects.
>>>> The refactored version skips the initialization of the generic flow
>>>> action attributes for tc actions, such as pedit, that allocate more
>>>> than one offload entry. This can cause potential issues for drivers mapping
>> flow action ids.
>>>>
>>>> Populate the generic flow action fields for all the flow action entries.
>>>>
>>>> Fixes: c54e1d920f04 ("flow_offload: add ops to tc_action_ops for flow
>>>> action
>>>> setup")
>>>> Signed-off-by: Oz Shlomo <ozsh@...dia.com>
>>>> Reviewed-by: Roi Dayan <roid@...dia.com>
>>>> ---
>>>> net/sched/cls_api.c | 17 +++++++++++++----
>>>> 1 file changed, 13 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index
>>>> 9bb4d3dcc994..d07c04096560 100644
>>>> --- a/net/sched/cls_api.c
>>>> +++ b/net/sched/cls_api.c
>>>> @@ -3533,7 +3533,7 @@ int tc_setup_action(struct flow_action
>> *flow_action,
>>>> 		    struct tc_action *actions[],
>>>> 		    struct netlink_ext_ack *extack) {
>>>> -	int i, j, index, err = 0;
>>>> +	int i, j, k, index, err = 0;
>>>> 	struct tc_action *act;
>>>>
>>>> 	BUILD_BUG_ON(TCA_ACT_HW_STATS_ANY !=
>> FLOW_ACTION_HW_STATS_ANY); @@
>>>> -3557,10 +3557,19 @@ int tc_setup_action(struct flow_action
>>>> *flow_action,
>>>> 		entry->hw_index = act->tcfa_index;
>>>> 		index = 0;
>>>> 		err = tc_setup_offload_act(act, entry, &index, extack);
>>>> -		if (!err)
>>>> -			j += index;
>>>> -		else
>>>> +		if (err)
>>>> 			goto err_out_locked;
>>>> +
>>>> +		/* initialize the generic parameters for actions that
>>>> +		 * allocate more than one offload entry per tc action
>>>> +		 */
>>>> +		for (k = 1; k < index ; k++) {
>>>> +			entry[k].hw_stats = tc_act_hw_stats(act->hw_stats);
>>>> +			entry[k].hw_index = act->tcfa_index;
>>> Thanks Oz for bringing this change to us, I think it makes sense for us when
>> the pedit action is offloaded as a single action.
>>> Just a tiny advice for your reference, maybe we can start assignment from k
>> = 0 and delete the first entry assignment above, then we will put all the
>> general assignment in this loop, it will be more clean, WDYT?
>>
>> If we do that then the hw_stats and hw_index parameters will not be
>> available to the offload_act_setup method.
>> AFAIU no tc action actually uses these values (so possibly no
>> regression) but perhaps it is better to leave them initialized.
> thanks for clarify about this, for the use of hw_index and hw_stats in tc_setup_offload_act, since we pass the act to function, I think we can get parameters from act if they are needed.
> What is your opinion?

I agree.
I will send v2.

>>
>>
>>
>>>> +		}
>>>> +
>>>> +		j += index;
>>>> +
>>>> 		spin_unlock_bh(&act->tcfa_lock);
>>>> 	}
>>>>
>>>> --
>>>> 1.8.3.1
>>>