On Sat 03 Apr 2021 at 01:13, Cong Wang <xiyou.wangc...@gmail.com> wrote:
> On Wed, Mar 31, 2021 at 9:41 AM Vlad Buslov <vla...@nvidia.com> wrote:
>>
>> Action init code increments reference counter when it changes an action.
>> This is the desired behavior for cls API which needs to obtain action
>> reference for every classifier that points to action. However, act API just
>> needs to change the action and releases the reference before returning.
>> This sequence breaks when the requested action doesn't exist, which causes
>> act API init code to create new action with specified index, but action is
>> still released before returning and is deleted (unless it was referenced
>> concurrently by cls API).
>
> Please also add a summary of how you fix it. From what I understand,
> it seems you just skip the refcnt put of successful cases?
Oops, I didn't regenerate patches after amending the commit message.
This should include the following paragraph:
Extend tcf_action_init() to accept 'init_res' array and initialize it with
action->ops->init() result. Refactor tcf_action_put_many() to also accept
such array and only put actions for which init result match provided value.
Modify tcf_action_add() to only put actions with init_res==0 instead of
unconditionally putting all actions when user set NLM_F_REPLACE netlink
message flag.
>
> One comment below.
>
>>
>> Fixes: cae422f379f3 ("net: sched: use reference counting action init")
>> Reported-by: Kumar Kartikeya Dwivedi <mem...@gmail.com>
>> Signed-off-by: Vlad Buslov <vla...@nvidia.com>
>> ---
>> include/net/act_api.h | 5 +++--
>> net/sched/act_api.c | 27 +++++++++++++++++----------
>> net/sched/cls_api.c | 9 +++++----
>> 3 files changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/net/act_api.h b/include/net/act_api.h
>> index 2bf3092ae7ec..312f0f6554a0 100644
>> --- a/include/net/act_api.h
>> +++ b/include/net/act_api.h
>> @@ -185,7 +185,7 @@ int tcf_action_exec(struct sk_buff *skb, struct
>> tc_action **actions,
>> int nr_actions, struct tcf_result *res);
>> int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr
>> *nla,
>> struct nlattr *est, char *name, int ovr, int bind,
>> - struct tc_action *actions[], size_t *attr_size,
>> + struct tc_action *actions[], int init_res[], size_t
>> *attr_size,
>> bool rtnl_held, struct netlink_ext_ack *extack);
>> struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla,
>> bool rtnl_held,
>> @@ -193,7 +193,8 @@ struct tc_action_ops *tc_action_load_ops(char *name,
>> struct nlattr *nla,
>> struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>> struct nlattr *nla, struct nlattr *est,
>> char *name, int ovr, int bind,
>> - struct tc_action_ops *ops, bool
>> rtnl_held,
>> + struct tc_action_ops *a_o, int *init_res,
>> + bool rtnl_held,
>> struct netlink_ext_ack *extack);
>> int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[], int
>> bind,
>> int ref, bool terse);
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index b919826939e0..eb20a75796d5 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -777,8 +777,11 @@ static int tcf_action_put(struct tc_action *p)
>> return __tcf_action_put(p, false);
>> }
>>
>> -/* Put all actions in this array, skip those NULL's. */
>> -static void tcf_action_put_many(struct tc_action *actions[])
>> +/* Put all actions in this array, skip those NULL's. If cond array is
>> provided
>> + * by caller, then only put actions that match.
>> + */
>> +static void tcf_action_put_many(struct tc_action *actions[], int *cond,
>> + int match)
>> {
>> int i;
>>
>> @@ -786,7 +789,7 @@ static void tcf_action_put_many(struct tc_action
>> *actions[])
>> struct tc_action *a = actions[i];
>> const struct tc_action_ops *ops;
>>
>> - if (!a)
>> + if (!a || (cond && cond[i] != match))
>
> This looks a bit odd. How about passing an array of action pointers which
> only contains those that need to be put?
I wanted to make it extensible with cond array instead of make every
user manually filter the action array before calling
tcf_action_put_many(). But I guess there is currently no need for that
and just extending tcf_action_add() with a loop to zero-out the pointers
for newly created actions will be clearer. Will change it in V2.
>
> Thanks.
