| Message ID | 20190927015157.20070-1-navid.emamdoost@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v3] nfp: abm: fix memory leak in nfp_abm_u32_knode_replace | expand |
> Updated other gotos to have correct errno returned, too. How do you think about to add a jump target here? > +++ b/drivers/net/ethernet/netronome/nfp/abm/cls.c > @@ -176,8 +176,10 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink, > u8 mask, val; > int err; > > - if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack)) > + if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack)) { > + err = -EOPNOTSUPP; > goto err_delete; > + } > > tos_off = proto == htons(ETH_P_IP) ? 16 : 20; - goto err_delete; + goto e_opnotsupp; > @@ -221,7 +227,7 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink, > +e_opnotsupp: + err = -EOPNOTSUPP; > err_delete: > nfp_abm_u32_knode_delete(alink, knode); > - return -EOPNOTSUPP; > + return err; > } > > static int nfp_abm_setup_tc_block_cb(enum tc_setup_type type, Can such a change variant be a bit nicer? Regards, Markus
From: Navid Emamdoost <navid.emamdoost@gmail.com> Date: Thu, 26 Sep 2019 20:51:46 -0500 > In nfp_abm_u32_knode_replace if the allocation for match fails it should > go to the error handling instead of returning. Updated other gotos to > have correct errno returned, too. > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> Applied.
On Fri, 27 Sep 2019 14:12:42 +0200, Markus Elfring wrote: > > Updated other gotos to have correct errno returned, too. > > How do you think about to add a jump target here? > > > > +++ b/drivers/net/ethernet/netronome/nfp/abm/cls.c > > @@ -176,8 +176,10 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink, > > u8 mask, val; > > int err; > > > > - if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack)) > > + if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack)) { > > + err = -EOPNOTSUPP; > > goto err_delete; > > + } > > > > tos_off = proto == htons(ETH_P_IP) ? 16 : 20; > > - goto err_delete; > + goto e_opnotsupp; > > > > @@ -221,7 +227,7 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink, > > > > +e_opnotsupp: > + err = -EOPNOTSUPP; > > > err_delete: > > nfp_abm_u32_knode_delete(alink, knode); > > - return -EOPNOTSUPP; > > + return err; > > } > > > > static int nfp_abm_setup_tc_block_cb(enum tc_setup_type type, > > > Can such a change variant be a bit nicer? Definitely not. Looks good as is, thanks Navid!
>> Can such a change variant be a bit nicer? > > Definitely not. > > Looks good as is, thanks Navid! I find it interesting how the software development opinions are different also in this use case for the implementation of correct and efficient exception handling. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=f1f2f614d535564992f32e720739cb53cf03489f#n450 Regards, Markus
On Thu, 26 Sep 2019 20:51:46 -0500, Navid Emamdoost wrote: > In nfp_abm_u32_knode_replace if the allocation for match fails it should > go to the error handling instead of returning. Updated other gotos to > have correct errno returned, too. > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > --- > Changes in v2: > - Reused err variable for erorr value returning. > Changes in v3: > - Fix the err value in the first goto, and fix subject prefix. Ugh damn this. Apparently this "fix" has made the news: https://news.softpedia.com/news/canonical-releases-major-kernel-security-update-for-ubuntu-19-10-and-18-04-lts-528433.shtml https://nvd.nist.gov/vuln/detail/CVE-2019-19076 and (a) it would be a damn control path, root-only memory leak, but also (b) upon closer inspection there is no leak here at all! We don't need to delete the entry if we failed to allocate it... The delete path is in case the entry for the handle is changed, but if we're trying to allocate one anew there can't be any on the list. Congratulations to whoever classified this as a security fix. I will send a revert, and go ask for the CVE to be marked invalid. What a waste of time. I should have paid more attention :/ > diff --git a/drivers/net/ethernet/netronome/nfp/abm/cls.c b/drivers/net/ethernet/netronome/nfp/abm/cls.c > index 23ebddfb9532..9f8a1f69c0c4 100644 > --- a/drivers/net/ethernet/netronome/nfp/abm/cls.c > +++ b/drivers/net/ethernet/netronome/nfp/abm/cls.c > @@ -176,8 +176,10 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink, > u8 mask, val; > int err; > > - if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack)) > + if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack)) { > + err = -EOPNOTSUPP; > goto err_delete; > + } > > tos_off = proto == htons(ETH_P_IP) ? 16 : 20; > > @@ -198,14 +200,18 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink, > if ((iter->val & cmask) == (val & cmask) && > iter->band != knode->res->classid) { > NL_SET_ERR_MSG_MOD(extack, "conflict with already offloaded filter"); > + err = -EOPNOTSUPP; > goto err_delete; > } > } > > if (!match) { > match = kzalloc(sizeof(*match), GFP_KERNEL); > - if (!match) > - return -ENOMEM; > + if (!match) { > + err = -ENOMEM; > + goto err_delete; > + } > + > list_add(&match->list, &alink->dscp_map); > } > match->handle = knode->handle; > @@ -221,7 +227,7 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink, > > err_delete: > nfp_abm_u32_knode_delete(alink, knode); > - return -EOPNOTSUPP; > + return err; > } > > static int nfp_abm_setup_tc_block_cb(enum tc_setup_type type,
diff --git a/drivers/net/ethernet/netronome/nfp/abm/cls.c b/drivers/net/ethernet/netronome/nfp/abm/cls.c index 23ebddfb9532..9f8a1f69c0c4 100644 --- a/drivers/net/ethernet/netronome/nfp/abm/cls.c +++ b/drivers/net/ethernet/netronome/nfp/abm/cls.c @@ -176,8 +176,10 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink, u8 mask, val; int err; - if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack)) + if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack)) { + err = -EOPNOTSUPP; goto err_delete; + } tos_off = proto == htons(ETH_P_IP) ? 16 : 20; @@ -198,14 +200,18 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink, if ((iter->val & cmask) == (val & cmask) && iter->band != knode->res->classid) { NL_SET_ERR_MSG_MOD(extack, "conflict with already offloaded filter"); + err = -EOPNOTSUPP; goto err_delete; } } if (!match) { match = kzalloc(sizeof(*match), GFP_KERNEL); - if (!match) - return -ENOMEM; + if (!match) { + err = -ENOMEM; + goto err_delete; + } + list_add(&match->list, &alink->dscp_map); } match->handle = knode->handle; @@ -221,7 +227,7 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink, err_delete: nfp_abm_u32_knode_delete(alink, knode); - return -EOPNOTSUPP; + return err; } static int nfp_abm_setup_tc_block_cb(enum tc_setup_type type,
In nfp_abm_u32_knode_replace if the allocation for match fails it should go to the error handling instead of returning. Updated other gotos to have correct errno returned, too. Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> --- Changes in v2: - Reused err variable for erorr value returning. Changes in v3: - Fix the err value in the first goto, and fix subject prefix. --- drivers/net/ethernet/netronome/nfp/abm/cls.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)