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
| ||
|
Message-ID: <a3c4a135-672f-0f66-271c-a6071aca7c89@nvidia.com> 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 >>>
Powered by blists - more mailing lists