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

Date:   Tue, 19 Jul 2022 01:30:32 +0000
From:   Baowen Zheng <baowen.zheng@...igine.com>
To:     Oz Shlomo <ozsh@...dia.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 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?
>
>
>
>>> +		}
>>> +
>>> +		j += index;
>>> +
>>> 		spin_unlock_bh(&act->tcfa_lock);
>>> 	}
>>>
>>> --
>>> 1.8.3.1
>>