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

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

[Oz Shlomo]

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@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.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;
+		}
+
+		j += index;
+
 		spin_unlock_bh(&act->tcfa_lock);
 	}
 
-- 
1.8.3.1

RE: [PATCH net] net/sched: cls_api: Fix flow action initialization

[Baowen Zheng]

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@nvidia.com>
>Reviewed-by: Roi Dayan <roid@nvidia.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?
>+		}
>+
>+		j += index;
>+
> 		spin_unlock_bh(&act->tcfa_lock);
> 	}
>
>--
>1.8.3.1

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

[Oz Shlomo]

Hi Baoweng,

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@nvidia.com>
>> Reviewed-by: Roi Dayan <roid@nvidia.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.



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

RE: [PATCH net] net/sched: cls_api: Fix flow action initialization

[Baowen Zheng]

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@nvidia.com>
>>> Reviewed-by: Roi Dayan <roid@nvidia.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
>>

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

[Oz Shlomo]

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@nvidia.com>
>>>> Reviewed-by: Roi Dayan <roid@nvidia.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
>>>