| Message ID | 20230605070158.48403-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: sched: fix possible refcount leak in tc_chain_tmplt_add() | expand |
On Mon, Jun 05, 2023 at 03:01:58PM +0800, Hangyu Hua wrote: > try_module_get can be called in tcf_proto_lookup_ops. So if ops don't > implement the corresponding function we should call module_put to drop > the refcount. Hi Hangyu Hua, Is this correct even if try_module_get() is not called via tcf_proto_lookup_ops() ? > Fixes: 9f407f1768d3 ("net: sched: introduce chain templates") > Signed-off-by: Hangyu Hua <hbh25y@gmail.com> > --- > net/sched/cls_api.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index 2621550bfddc..92bfb892e638 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -2952,6 +2952,7 @@ static int tc_chain_tmplt_add(struct tcf_chain *chain, struct net *net, > return PTR_ERR(ops); > if (!ops->tmplt_create || !ops->tmplt_destroy || !ops->tmplt_dump) { > NL_SET_ERR_MSG(extack, "Chain templates are not supported with specified classifier"); > + module_put(ops->owner); > return -EOPNOTSUPP; > } > > -- > 2.34.1 > >
On 5/6/2023 20:31, Simon Horman wrote: > On Mon, Jun 05, 2023 at 03:01:58PM +0800, Hangyu Hua wrote: >> try_module_get can be called in tcf_proto_lookup_ops. So if ops don't >> implement the corresponding function we should call module_put to drop >> the refcount. > > Hi Hangyu Hua, > > Is this correct even if try_module_get() is > not called via tcf_proto_lookup_ops() ? > tcf_proto_lookup_ops will return error if try_module_get() is not called in tcf_proto_lookup_ops(). I am not sure what you mean? Thanks, Hangyu >> Fixes: 9f407f1768d3 ("net: sched: introduce chain templates") >> Signed-off-by: Hangyu Hua <hbh25y@gmail.com> >> --- >> net/sched/cls_api.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >> index 2621550bfddc..92bfb892e638 100644 >> --- a/net/sched/cls_api.c >> +++ b/net/sched/cls_api.c >> @@ -2952,6 +2952,7 @@ static int tc_chain_tmplt_add(struct tcf_chain *chain, struct net *net, >> return PTR_ERR(ops); >> if (!ops->tmplt_create || !ops->tmplt_destroy || !ops->tmplt_dump) { >> NL_SET_ERR_MSG(extack, "Chain templates are not supported with specified classifier"); >> + module_put(ops->owner); >> return -EOPNOTSUPP; >> } >> >> -- >> 2.34.1 >> >>
On Mon, Jun 05, 2023 at 03:01:58PM +0800, Hangyu Hua wrote: > try_module_get can be called in tcf_proto_lookup_ops. So if ops don't > implement the corresponding function we should call module_put to drop > the refcount. > Code seems reasonable. But commit message is pretty hard to understand. Please, replace "corresponding" with "required". Also change the first sentence, do not use "can". From what I see, successful execution of tcf_proto_lookup_ops always means we now hold reference to module. CC me in v2, I'll give you Reviewed-by. > Fixes: 9f407f1768d3 ("net: sched: introduce chain templates") > Signed-off-by: Hangyu Hua <hbh25y@gmail.com> > --- > net/sched/cls_api.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index 2621550bfddc..92bfb892e638 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -2952,6 +2952,7 @@ static int tc_chain_tmplt_add(struct tcf_chain *chain, struct net *net, > return PTR_ERR(ops); > if (!ops->tmplt_create || !ops->tmplt_destroy || !ops->tmplt_dump) { > NL_SET_ERR_MSG(extack, "Chain templates are not supported with specified classifier"); > + module_put(ops->owner); > return -EOPNOTSUPP; > } > > -- > 2.34.1 > >
On Tue, Jun 06, 2023 at 10:13:44AM +0800, Hangyu Hua wrote: > On 5/6/2023 20:31, Simon Horman wrote: > > On Mon, Jun 05, 2023 at 03:01:58PM +0800, Hangyu Hua wrote: > > > try_module_get can be called in tcf_proto_lookup_ops. So if ops don't > > > implement the corresponding function we should call module_put to drop > > > the refcount. > > > > Hi Hangyu Hua, > > > > Is this correct even if try_module_get() is > > not called via tcf_proto_lookup_ops() ? > > > > tcf_proto_lookup_ops will return error if try_module_get() is not called in > tcf_proto_lookup_ops(). I am not sure what you mean? Thanks, I think that answers my question. My concern was if there is a situation where module_put() is now called, but there was not in fact a reference that should be released. ...
Mon, Jun 05, 2023 at 09:01:58AM CEST, hbh25y@gmail.com wrote: >try_module_get can be called in tcf_proto_lookup_ops. So if ops don't >implement the corresponding function we should call module_put to drop >the refcount. Who's "we"? Use imperative mood. Tell the codebase what to do, what to change, etc. Code-wise, this is fine. Please fix the patch description. > >Fixes: 9f407f1768d3 ("net: sched: introduce chain templates") >Signed-off-by: Hangyu Hua <hbh25y@gmail.com> >--- > net/sched/cls_api.c | 1 + > 1 file changed, 1 insertion(+) > >diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >index 2621550bfddc..92bfb892e638 100644 >--- a/net/sched/cls_api.c >+++ b/net/sched/cls_api.c >@@ -2952,6 +2952,7 @@ static int tc_chain_tmplt_add(struct tcf_chain *chain, struct net *net, > return PTR_ERR(ops); > if (!ops->tmplt_create || !ops->tmplt_destroy || !ops->tmplt_dump) { > NL_SET_ERR_MSG(extack, "Chain templates are not supported with specified classifier"); >+ module_put(ops->owner); > return -EOPNOTSUPP; > } > >-- >2.34.1 >
On 6/6/2023 16:53, Larysa Zaremba wrote: > On Mon, Jun 05, 2023 at 03:01:58PM +0800, Hangyu Hua wrote: >> try_module_get can be called in tcf_proto_lookup_ops. So if ops don't >> implement the corresponding function we should call module_put to drop >> the refcount. >> > > Code seems reasonable. But commit message is pretty hard to understand. > Please, replace "corresponding" with "required". > Also change the first sentence, do not use "can". From what I see, successful > execution of tcf_proto_lookup_ops always means we now hold reference to module. > > CC me in v2, I'll give you Reviewed-by. > I apologize for my incorrect English expression. I will send a v2 later. Thanks, Hangyu >> Fixes: 9f407f1768d3 ("net: sched: introduce chain templates") >> Signed-off-by: Hangyu Hua <hbh25y@gmail.com> >> --- >> net/sched/cls_api.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >> index 2621550bfddc..92bfb892e638 100644 >> --- a/net/sched/cls_api.c >> +++ b/net/sched/cls_api.c >> @@ -2952,6 +2952,7 @@ static int tc_chain_tmplt_add(struct tcf_chain *chain, struct net *net, >> return PTR_ERR(ops); >> if (!ops->tmplt_create || !ops->tmplt_destroy || !ops->tmplt_dump) { >> NL_SET_ERR_MSG(extack, "Chain templates are not supported with specified classifier"); >> + module_put(ops->owner); >> return -EOPNOTSUPP; >> } >> >> -- >> 2.34.1 >> >>
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 2621550bfddc..92bfb892e638 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -2952,6 +2952,7 @@ static int tc_chain_tmplt_add(struct tcf_chain *chain, struct net *net, return PTR_ERR(ops); if (!ops->tmplt_create || !ops->tmplt_destroy || !ops->tmplt_dump) { NL_SET_ERR_MSG(extack, "Chain templates are not supported with specified classifier"); + module_put(ops->owner); return -EOPNOTSUPP; }
try_module_get can be called in tcf_proto_lookup_ops. So if ops don't implement the corresponding function we should call module_put to drop the refcount. Fixes: 9f407f1768d3 ("net: sched: introduce chain templates") Signed-off-by: Hangyu Hua <hbh25y@gmail.com> --- net/sched/cls_api.c | 1 + 1 file changed, 1 insertion(+)