In nfp_abm_u32_knode_replace if the allocation for match fails it should
go to the error handling instead of returning.
Signed-off-by: Navid Emamdoost <[email protected]>
---
drivers/net/ethernet/netronome/nfp/abm/cls.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/abm/cls.c b/drivers/net/ethernet/netronome/nfp/abm/cls.c
index 23ebddfb9532..32eaab99d96c 100644
--- a/drivers/net/ethernet/netronome/nfp/abm/cls.c
+++ b/drivers/net/ethernet/netronome/nfp/abm/cls.c
@@ -174,7 +174,7 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink,
struct nfp_abm_u32_match *match = NULL, *iter;
unsigned int tos_off;
u8 mask, val;
- int err;
+ int err, ret = -EOPNOTSUPP;
if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack))
goto err_delete;
@@ -204,8 +204,11 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink,
if (!match) {
match = kzalloc(sizeof(*match), GFP_KERNEL);
- if (!match)
- return -ENOMEM;
+ if (!match) {
+ ret = -ENOMEM;
+ goto err_delete;
+ }
+
list_add(&match->list, &alink->dscp_map);
}
match->handle = knode->handle;
@@ -221,7 +224,7 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink,
err_delete:
nfp_abm_u32_knode_delete(alink, knode);
- return -EOPNOTSUPP;
+ return ret;
}
static int nfp_abm_setup_tc_block_cb(enum tc_setup_type type,
--
2.17.1
On Wed, 25 Sep 2019 13:34: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.
>
> Signed-off-by: Navid Emamdoost <[email protected]>
> ---
> drivers/net/ethernet/netronome/nfp/abm/cls.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/netronome/nfp/abm/cls.c b/drivers/net/ethernet/netronome/nfp/abm/cls.c
> index 23ebddfb9532..32eaab99d96c 100644
> --- a/drivers/net/ethernet/netronome/nfp/abm/cls.c
> +++ b/drivers/net/ethernet/netronome/nfp/abm/cls.c
> @@ -174,7 +174,7 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink,
> struct nfp_abm_u32_match *match = NULL, *iter;
> unsigned int tos_off;
> u8 mask, val;
> - int err;
> + int err, ret = -EOPNOTSUPP;
You can use the err variable for the return. Please don't break the
reverse christmas tree ordering. Please initialize the err variable
in the branch where failure occurred, not at the start of the function.
> if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack))
> goto err_delete;
> @@ -204,8 +204,11 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink,
>
> if (!match) {
> match = kzalloc(sizeof(*match), GFP_KERNEL);
> - if (!match)
> - return -ENOMEM;
> + if (!match) {
> + ret = -ENOMEM;
> + goto err_delete;
> + }
> +
> list_add(&match->list, &alink->dscp_map);
> }
> match->handle = knode->handle;
> @@ -221,7 +224,7 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink,
>
> err_delete:
> nfp_abm_u32_knode_delete(alink, knode);
> - return -EOPNOTSUPP;
> + return ret;
> }
>
> 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.
Signed-off-by: Navid Emamdoost <[email protected]>
---
Changes in v2:
- Reused err variable for erorr value returning.
---
drivers/net/ethernet/netronome/nfp/abm/cls.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/abm/cls.c b/drivers/net/ethernet/netronome/nfp/abm/cls.c
index 23ebddfb9532..b0cb9d201f7d 100644
--- a/drivers/net/ethernet/netronome/nfp/abm/cls.c
+++ b/drivers/net/ethernet/netronome/nfp/abm/cls.c
@@ -198,14 +198,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 +225,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,
--
2.17.1
On Wed, 25 Sep 2019 21:22:35 -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.
>
> Signed-off-by: Navid Emamdoost <[email protected]>
> ---
> Changes in v2:
> - Reused err variable for erorr value returning.
Thanks, there's another goto up top. And I think subject prefix could
be "nfp: abm:", perhaps?
> drivers/net/ethernet/netronome/nfp/abm/cls.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/netronome/nfp/abm/cls.c b/drivers/net/ethernet/netronome/nfp/abm/cls.c
> index 23ebddfb9532..b0cb9d201f7d 100644
> --- a/drivers/net/ethernet/netronome/nfp/abm/cls.c
> +++ b/drivers/net/ethernet/netronome/nfp/abm/cls.c
> @@ -198,14 +198,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 +225,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 <[email protected]>
---
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(-)
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,
--
2.17.1
On Wed, Sep 25, 2019 at 09:53:14PM -0700, Jakub Kicinski wrote:
> On Wed, 25 Sep 2019 21:22:35 -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.
> >
> > Signed-off-by: Navid Emamdoost <[email protected]>
> > ---
> > Changes in v2:
> > - Reused err variable for erorr value returning.
>
> Thanks, there's another goto up top. And I think subject prefix could
> be "nfp: abm:", perhaps?
>
Thanks, v3 was sent which fixes this.
Navid.
> > drivers/net/ethernet/netronome/nfp/abm/cls.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/netronome/nfp/abm/cls.c b/drivers/net/ethernet/netronome/nfp/abm/cls.c
> > index 23ebddfb9532..b0cb9d201f7d 100644
> > --- a/drivers/net/ethernet/netronome/nfp/abm/cls.c
> > +++ b/drivers/net/ethernet/netronome/nfp/abm/cls.c
> > @@ -198,14 +198,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 +225,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,
>
> 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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
> ---
> 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,