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]

Message-ID: <DM5PR1301MB2172BA76D9BAEADF9A40D11CE78C9@DM5PR1301MB2172.namprd13.prod.outlook.com>
Date:   Mon, 18 Jul 2022 01:40:13 +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>
Subject: RE: [PATCH net] net/sched: cls_api: Fix flow action initialization

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?
>+		}
>+
>+		j += index;
>+
> 		spin_unlock_bh(&act->tcfa_lock);
> 	}
>
>--
>1.8.3.1