Re: [PATCH 09/14] net: sched: don't release reference on action overwrite
On Sat 19 May 2018 at 21:52, Marcelo Ricardo Leitner wrote: > On Mon, May 14, 2018 at 05:27:10PM +0300, Vlad Buslov wrote: >> Return from action init function with reference to action taken, >> even when overwriting existing action. > > Isn't this patch necessary before patch 7, to not break things up? > AFAICU after patchset 7 it assumes the action init function is already > behaving like this. I have re-split these patches for V2.
Re: [PATCH 09/14] net: sched: don't release reference on action overwrite
On Mon, May 14, 2018 at 05:27:10PM +0300, Vlad Buslov wrote: > Return from action init function with reference to action taken, > even when overwriting existing action. Isn't this patch necessary before patch 7, to not break things up? AFAICU after patchset 7 it assumes the action init function is already behaving like this.
Re: [PATCH 09/14] net: sched: don't release reference on action overwrite
Wed, May 16, 2018 at 09:47:32AM CEST, vla...@mellanox.com wrote: > >On Wed 16 May 2018 at 07:43, Jiri Pirko wrote: >> Mon, May 14, 2018 at 04:27:10PM CEST, vla...@mellanox.com wrote: >>>Return from action init function with reference to action taken, >>>even when overwriting existing action. >>> >>>Action init API initializes its fourth argument (pointer to pointer to >>>tc action) to either existing action with same index or newly created >>>action. In case of existing index(and bind argument is zero), init >>>function returns without incrementing action reference counter. Caller >>>of action init then proceeds working with action without actually >>>holding reference to it. This means that action could be deleted >>>concurrently. To prevent such scenario this patch changes action init >> >> Be imperative to the codebase in the patch description. >> >> >>>behavior to always take reference to action before returning >>>successfully. >> >> Where's the balance? Who does the release instead? I'm probably missing >> something. > >I've resplit these patches for V2 to always do take/release in same >patch. Good. Thanks. > >> >>> >>>Signed-off-by: Vlad Buslov >>>--- >>> net/sched/act_bpf.c| 8 >>> net/sched/act_connmark.c | 5 +++-- >>> net/sched/act_csum.c | 8 >>> net/sched/act_gact.c | 5 +++-- >>> net/sched/act_ife.c| 12 +--- >>> net/sched/act_ipt.c| 5 +++-- >>> net/sched/act_mirred.c | 5 ++--- >>> net/sched/act_nat.c| 5 +++-- >>> net/sched/act_pedit.c | 5 +++-- >>> net/sched/act_police.c | 8 +++- >>> net/sched/act_sample.c | 8 +++- >>> net/sched/act_simple.c | 5 +++-- >>> net/sched/act_skbedit.c| 5 +++-- >>> net/sched/act_skbmod.c | 8 +++- >>> net/sched/act_tunnel_key.c | 8 +++- >>> net/sched/act_vlan.c | 8 +++- >>> 16 files changed, 51 insertions(+), 57 deletions(-) >>> >>>diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c >>>index 5d95c43..5554bf7 100644 >>>--- a/net/sched/act_bpf.c >>>+++ b/net/sched/act_bpf.c >>>@@ -311,9 +311,10 @@ static int tcf_bpf_init(struct net *net, struct nlattr >>>*nla, >>> if (bind) >>> return 0; >>> >>>-tcf_idr_release(*act, bind); >>>-if (!replace) >>>+if (!replace) { >>>+tcf_idr_release(*act, bind); >>> return -EEXIST; >>>+} >>> } >>> >>> is_bpf = tb[TCA_ACT_BPF_OPS_LEN] && tb[TCA_ACT_BPF_OPS]; >> >> [...] >
Re: [PATCH 09/14] net: sched: don't release reference on action overwrite
On Wed 16 May 2018 at 07:43, Jiri Pirko wrote: > Mon, May 14, 2018 at 04:27:10PM CEST, vla...@mellanox.com wrote: >>Return from action init function with reference to action taken, >>even when overwriting existing action. >> >>Action init API initializes its fourth argument (pointer to pointer to >>tc action) to either existing action with same index or newly created >>action. In case of existing index(and bind argument is zero), init >>function returns without incrementing action reference counter. Caller >>of action init then proceeds working with action without actually >>holding reference to it. This means that action could be deleted >>concurrently. To prevent such scenario this patch changes action init > > Be imperative to the codebase in the patch description. > > >>behavior to always take reference to action before returning >>successfully. > > Where's the balance? Who does the release instead? I'm probably missing > something. I've resplit these patches for V2 to always do take/release in same patch. > >> >>Signed-off-by: Vlad Buslov >>--- >> net/sched/act_bpf.c| 8 >> net/sched/act_connmark.c | 5 +++-- >> net/sched/act_csum.c | 8 >> net/sched/act_gact.c | 5 +++-- >> net/sched/act_ife.c| 12 +--- >> net/sched/act_ipt.c| 5 +++-- >> net/sched/act_mirred.c | 5 ++--- >> net/sched/act_nat.c| 5 +++-- >> net/sched/act_pedit.c | 5 +++-- >> net/sched/act_police.c | 8 +++- >> net/sched/act_sample.c | 8 +++- >> net/sched/act_simple.c | 5 +++-- >> net/sched/act_skbedit.c| 5 +++-- >> net/sched/act_skbmod.c | 8 +++- >> net/sched/act_tunnel_key.c | 8 +++- >> net/sched/act_vlan.c | 8 +++- >> 16 files changed, 51 insertions(+), 57 deletions(-) >> >>diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c >>index 5d95c43..5554bf7 100644 >>--- a/net/sched/act_bpf.c >>+++ b/net/sched/act_bpf.c >>@@ -311,9 +311,10 @@ static int tcf_bpf_init(struct net *net, struct nlattr >>*nla, >> if (bind) >> return 0; >> >>- tcf_idr_release(*act, bind); >>- if (!replace) >>+ if (!replace) { >>+ tcf_idr_release(*act, bind); >> return -EEXIST; >>+ } >> } >> >> is_bpf = tb[TCA_ACT_BPF_OPS_LEN] && tb[TCA_ACT_BPF_OPS]; > > [...]
Re: [PATCH 09/14] net: sched: don't release reference on action overwrite
Mon, May 14, 2018 at 04:27:10PM CEST, vla...@mellanox.com wrote: >Return from action init function with reference to action taken, >even when overwriting existing action. > >Action init API initializes its fourth argument (pointer to pointer to >tc action) to either existing action with same index or newly created >action. In case of existing index(and bind argument is zero), init >function returns without incrementing action reference counter. Caller >of action init then proceeds working with action without actually >holding reference to it. This means that action could be deleted >concurrently. To prevent such scenario this patch changes action init Be imperative to the codebase in the patch description. >behavior to always take reference to action before returning >successfully. Where's the balance? Who does the release instead? I'm probably missing something. > >Signed-off-by: Vlad Buslov >--- > net/sched/act_bpf.c| 8 > net/sched/act_connmark.c | 5 +++-- > net/sched/act_csum.c | 8 > net/sched/act_gact.c | 5 +++-- > net/sched/act_ife.c| 12 +--- > net/sched/act_ipt.c| 5 +++-- > net/sched/act_mirred.c | 5 ++--- > net/sched/act_nat.c| 5 +++-- > net/sched/act_pedit.c | 5 +++-- > net/sched/act_police.c | 8 +++- > net/sched/act_sample.c | 8 +++- > net/sched/act_simple.c | 5 +++-- > net/sched/act_skbedit.c| 5 +++-- > net/sched/act_skbmod.c | 8 +++- > net/sched/act_tunnel_key.c | 8 +++- > net/sched/act_vlan.c | 8 +++- > 16 files changed, 51 insertions(+), 57 deletions(-) > >diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c >index 5d95c43..5554bf7 100644 >--- a/net/sched/act_bpf.c >+++ b/net/sched/act_bpf.c >@@ -311,9 +311,10 @@ static int tcf_bpf_init(struct net *net, struct nlattr >*nla, > if (bind) > return 0; > >- tcf_idr_release(*act, bind); >- if (!replace) >+ if (!replace) { >+ tcf_idr_release(*act, bind); > return -EEXIST; >+ } > } > > is_bpf = tb[TCA_ACT_BPF_OPS_LEN] && tb[TCA_ACT_BPF_OPS]; [...]