Re: [PATCH 07/14] net: sched: use reference counting action init

From: Jiri Pirko
Date: Tue May 15 2018 - 13:39:29 EST

Tue, May 15, 2018 at 01:41:45PM CEST, vladbu@xxxxxxxxxxxx wrote:
>On Tue 15 May 2018 at 11:39, Jiri Pirko <jiri@xxxxxxxxxxx> wrote:
>> Tue, May 15, 2018 at 01:32:51PM CEST, vladbu@xxxxxxxxxxxx wrote:
>>>On Tue 15 May 2018 at 11:24, Jiri Pirko <jiri@xxxxxxxxxxx> wrote:
>>>> Mon, May 14, 2018 at 04:27:08PM CEST, vladbu@xxxxxxxxxxxx wrote:
>>>>>Change action API to assume that action init function always takes
>>>>>reference to action, even when overwriting existing action. This is
>>>>>necessary because action API continues to use action pointer after init
>>>>>function is done. At this point action becomes accessible for concurrent
>>>>>modifications so user must always hold reference to it.
>>>>>Implement helper put list function to atomically release list of actions
>>>>>after action API init code is done using them.
>>>>>Signed-off-by: Vlad Buslov <vladbu@xxxxxxxxxxxx>
>>>>> net/sched/act_api.c | 38 +++++++++++++++++---------------------
>>>>> 1 file changed, 17 insertions(+), 21 deletions(-)
>>>> [...]
>>>>>@@ -1196,8 +1190,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
>>>>> return ret;
>>>>> }
>>>>> err:
>>>>>- if (event != RTM_GETACTION)
>>>> Howcome you do this for RTM_GETACTION now too? Where is the related
>>>> "get"?
>>>In patch 5. There is always a possibility of concurrent delete without
>>>rtnl lock so all usages of action pointers were converted to hold
>>>reference to action.
>> So that means that if you run kernel in between, with patch 5 but
>> without patch 7 and you do RTM_GETACTION, you leak a reference, right?

That is an issue. You need to make sure that the code is without bugs
like this after every applied patch. You need to make sure the code is