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

[Jiri Pirko]

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?
>
>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
bisectable.

Thanks.