From: Cong Wang <xiyou.wangcong@gmail.com>
To: Vlad Buslov <vladbu@nvidia.com>
Cc: "Linux Kernel Network Developers" <netdev@vger.kernel.org>,
"Kumar Kartikeya Dwivedi" <memxor@gmail.com>,
"David Miller" <davem@davemloft.net>,
"Jamal Hadi Salim" <jhs@mojatatu.com>,
"Jiri Pirko" <jiri@resnulli.us>,
"Jakub Kicinski" <kuba@kernel.org>,
"Toke Høiland-Jørgensen" <toke@redhat.com>
Subject: Re: [PATCH RFC 1/4] net: sched: fix action overwrite reference counting
Date: Fri, 2 Apr 2021 15:13:02 -0700 [thread overview]
Message-ID: <CAM_iQpX85rHgUMvwQskEFqa+MRtv5Y_+ZbMnds9xxmaKP=qiOg@mail.gmail.com> (raw)
In-Reply-To: <20210331164012.28653-2-vladbu@nvidia.com>
On Wed, Mar 31, 2021 at 9:41 AM Vlad Buslov <vladbu@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?
One comment below.
>
> Fixes: cae422f379f3 ("net: sched: use reference counting action init")
> Reported-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> Signed-off-by: Vlad Buslov <vladbu@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?
Thanks.
next prev parent reply other threads:[~2021-04-02 22:13 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-31 16:40 [PATCH RFC 0/4] Action initalization fixes Vlad Buslov
2021-03-31 16:40 ` [PATCH RFC 1/4] net: sched: fix action overwrite reference counting Vlad Buslov
2021-04-02 22:13 ` Cong Wang [this message]
2021-04-03 9:25 ` Vlad Buslov
2021-03-31 16:40 ` [PATCH RFC 2/4] net: sched: fix err handler in tcf_action_init() Vlad Buslov
2021-04-02 23:14 ` Cong Wang
2021-04-03 10:01 ` Vlad Buslov
2021-04-05 22:56 ` Cong Wang
2021-04-06 19:35 ` Vlad Buslov
2021-03-31 16:40 ` [PATCH RFC 3/4] tc-testing: add simple action test to verify batch add cleanup Vlad Buslov
2021-03-31 16:40 ` [PATCH RFC 4/4] tc-testing: add simple action test to verify batch change cleanup Vlad Buslov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAM_iQpX85rHgUMvwQskEFqa+MRtv5Y_+ZbMnds9xxmaKP=qiOg@mail.gmail.com' \
--to=xiyou.wangcong@gmail.com \
--cc=davem@davemloft.net \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=kuba@kernel.org \
--cc=memxor@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=toke@redhat.com \
--cc=vladbu@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).