| 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
| ||
|
Date: Wed, 27 Oct 2021 16:42:17 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Vlad Buslov <vladbu@...dia.com>
Cc: netdev@...r.kernel.org, Jamal Hadi Salim <jhs@...atatu.com>,
Roi Dayan <roid@...dia.com>,
Cong Wang <xiyou.wangcong@...il.com>,
Jiri Pirko <jiri@...lanox.com>,
Baowen Zheng <notifications@...hub.com>,
Louis Peens <louis.peens@...igine.com>,
oss-drivers@...igine.com, Baowen Zheng <baowen.zheng@...igine.com>
Subject: Re: [RFC/PATCH net-next v2 2/5] flow_offload: allow user to offload
tc action to net device
Hi Vlad,
On Fri, Oct 01, 2021 at 07:20:52PM +0300, Vlad Buslov wrote:
>
> On Fri 01 Oct 2021 at 14:32, Simon Horman <simon.horman@...igine.com> wrote:
> > From: Baowen Zheng <baowen.zheng@...igine.com>
> >
> > Use flow_indr_dev_register/flow_indr_dev_setup_offload to
> > offload tc action.
...
Thanks for your review and sorry for the delay in responding.
I believe that at this point we have addressed most of the points
your raised and plan to post a v3 shortly.
At this point I'd like to relay some responses from Baowen who
has been working on addressing your review.
> > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> > index 3961461d9c8b..bf76baca579d 100644
> > --- a/include/net/flow_offload.h
> > +++ b/include/net/flow_offload.h
> > @@ -148,6 +148,10 @@ enum flow_action_id {
> > FLOW_ACTION_MPLS_MANGLE,
> > FLOW_ACTION_GATE,
> > FLOW_ACTION_PPPOE_PUSH,
> > + FLOW_ACTION_PEDIT, /* generic action type of pedit action for action
> > + * offload, it will be different type when adding
> > + * tc actions
> > + */
>
> This doesn't seem to be used anywhere in the series (it is set by
> flow_action_init() but never read). It is also confusing to add another
> id for pedit when FLOW_ACTION_{ADD|MANGLE} already exists in same enum.
Yes, agreed. It is to be used in driver, but since we do not use it by now
we will drop it from this patch.
...
> > +/* SKIP_HW and SKIP_SW are mutually exclusive flags. */
> > +static inline bool tc_act_flags_valid(u32 flags)
> > +{
> > + flags &= TCA_ACT_FLAGS_SKIP_HW | TCA_ACT_FLAGS_SKIP_SW;
> > + if (!(flags ^ (TCA_ACT_FLAGS_SKIP_HW | TCA_ACT_FLAGS_SKIP_SW)))
> > + return false;
>
> Can be simplified to just:
>
> return flags ^ (TCA_ACT_FLAGS_SKIP_HW | TCA_ACT_FLAGS_SKIP_SW);
Thanks, we will include that in v3.
...
> > +#define TCA_ACT_FLAGS_SKIP_HW (1 << 1) /* don't offload action to HW */
> > +#define TCA_ACT_FLAGS_SKIP_SW (1 << 2) /* don't use action in SW */
> > +#define TCA_ACT_FLAGS_IN_HW (1 << 3) /* action is offloaded to HW */
> > +#define TCA_ACT_FLAGS_NOT_IN_HW (1 << 4) /* action isn't offloaded to HW */
> > +#define TCA_ACT_FLAGS_VERBOSE (1 << 5) /* verbose logging */
>
> Doesn't seem to be used anywhere.
Thanks, we will drop this from v3.
...
> > @@ -145,6 +158,7 @@ static int __tcf_action_put(struct tc_action *p, bool bind)
> > if (refcount_dec_and_mutex_lock(&p->tcfa_refcnt, &idrinfo->lock)) {
> > if (bind)
> > atomic_dec(&p->tcfa_bindcnt);
> > + tcf_action_offload_del(p);
> > idr_remove(&idrinfo->action_idr, p->tcfa_index);
> > mutex_unlock(&idrinfo->lock);
>
> I'm curious whether it is required to call tcf_action_offload_del()
> inside idrinfo->lock critical section here.
Thanks for bringing this to our attention.
We'll move action offload delete out of the critical section in v3.
...
> > @@ -1059,6 +1088,219 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> > return ERR_PTR(err);
> > }
> >
> > +static int flow_action_init(struct flow_offload_action *fl_action,
> > + struct tc_action *act,
> > + enum flow_act_command cmd,
> > + struct netlink_ext_ack *extack)
> > +{
> > + int err;
> > +
> > + if (!fl_action)
> > + return -EINVAL;
> > +
> > + fl_action->extack = extack;
> > + fl_action->command = cmd;
> > + fl_action->index = act->tcfa_index;
> > +
> > + if (is_tcf_gact_ok(act)) {
> > + fl_action->id = FLOW_ACTION_ACCEPT;
> > + } else if (is_tcf_gact_shot(act)) {
> > + fl_action->id = FLOW_ACTION_DROP;
...
> Why is this function needed? It sets flow_offload_action->id, but
> similar value is also set in flow_offload_action->entries[]->id.
We set the flow_offload_action->entries only in the action add/replace
case. So for action delete/stats we need the id in the flow_offload_action.
This is similar to how classifier offload is handled.
> > +
> > +static void flow_action_update_hw(struct tc_action *act,
> > + u32 hw_count,
> > + enum flow_act_hw_oper cmd)
>
> Enum flow_act_hw_oper is defined together with similar-ish enum
> flow_act_command and only instance of flow_act_hw_oper is called "cmd"
> which is confusing. More thoughts in next comment.
>
> > +{
> > + if (!act)
> > + return;
> > +
> > + switch (cmd) {
> > + case FLOW_ACT_HW_ADD:
> > + act->in_hw_count = hw_count;
> > + break;
> > + case FLOW_ACT_HW_UPDATE:
> > + act->in_hw_count += hw_count;
> > + break;
> > + case FLOW_ACT_HW_DEL:
> > + act->in_hw_count = act->in_hw_count > hw_count ?
> > + act->in_hw_count - hw_count : 0;
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + if (act->in_hw_count) {
> > + act->tcfa_flags &= ~TCA_ACT_FLAGS_NOT_IN_HW;
> > + act->tcfa_flags |= TCA_ACT_FLAGS_IN_HW;
> > + } else {
> > + act->tcfa_flags &= ~TCA_ACT_FLAGS_IN_HW;
> > + act->tcfa_flags |= TCA_ACT_FLAGS_NOT_IN_HW;
> > + }
>
> I guess this could be just split to three one-liners for add/update/del,
> if not for common code to update flags. Such simplification would also
> probably remove the need for flow_act_hw_oper enum. I know I suggested
> mimicking similar classifier infrastructure, but after thinking about it
> some more maybe it would be better to parse in_hw_count to determine
> whether action is in hardware (and either only set {not_}in_hw flags
> before dumping to user space or get rid of them altogether)? After all,
> it seems that having both in classifier API is just due to historic
> reasons - in_hw flags were added first and couldn't be removed in order
> not to break userland after in_hw_count was implemented. WDYT?
Thanks, this is a good point.
In v3 we will drop in_hw and not_in_hw, and allow user-space to
derive them, if needed, from in_hw_count.
...
> This ~400 lines patch is hard to properly review. I would suggest
> splitting it.
Thanks, we've split this into three patches in v3.
Hopefully the result is a bit easier on the eyes.
Powered by blists - more mailing lists