* [PATCH] net: flow_offload: fix memory leak in nfp_abm_u32_knode_replace
@ 2019-09-25 18:34 Navid Emamdoost
2019-09-26 1:28 ` Jakub Kicinski
0 siblings, 1 reply; 11+ messages in thread
From: Navid Emamdoost @ 2019-09-25 18:34 UTC (permalink / raw)
Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Jakub Kicinski,
David S. Miller, Pablo Neira Ayuso, Colin Ian King, oss-drivers,
netdev, linux-kernel
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 <navid.emamdoost@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] net: flow_offload: fix memory leak in nfp_abm_u32_knode_replace
2019-09-25 18:34 [PATCH] net: flow_offload: fix memory leak in nfp_abm_u32_knode_replace Navid Emamdoost
@ 2019-09-26 1:28 ` Jakub Kicinski
2019-09-26 2:22 ` [PATCH v2] " Navid Emamdoost
0 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2019-09-26 1:28 UTC (permalink / raw)
To: Navid Emamdoost
Cc: emamd001, smccaman, kjlu, David S. Miller, Pablo Neira Ayuso,
Colin Ian King, oss-drivers, netdev, linux-kernel
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 <navid.emamdoost@gmail.com>
> ---
> 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,
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] net: flow_offload: fix memory leak in nfp_abm_u32_knode_replace
2019-09-26 1:28 ` Jakub Kicinski
@ 2019-09-26 2:22 ` Navid Emamdoost
2019-09-26 4:53 ` Jakub Kicinski
0 siblings, 1 reply; 11+ messages in thread
From: Navid Emamdoost @ 2019-09-26 2:22 UTC (permalink / raw)
To: jakub.kicinski
Cc: emamd001, smccaman, kjlu, Navid Emamdoost, David S. Miller,
Pablo Neira Ayuso, John Hurley, Colin Ian King, oss-drivers,
netdev, linux-kernel
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 <navid.emamdoost@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] net: flow_offload: fix memory leak in nfp_abm_u32_knode_replace
2019-09-26 2:22 ` [PATCH v2] " Navid Emamdoost
@ 2019-09-26 4:53 ` Jakub Kicinski
2019-09-27 1:51 ` [PATCH v3] nfp: abm: " Navid Emamdoost
2019-09-27 2:26 ` [PATCH v2] net: flow_offload: " Navid Emamdoost
0 siblings, 2 replies; 11+ messages in thread
From: Jakub Kicinski @ 2019-09-26 4:53 UTC (permalink / raw)
To: Navid Emamdoost
Cc: emamd001, smccaman, kjlu, David S. Miller, Pablo Neira Ayuso,
John Hurley, Colin Ian King, oss-drivers, netdev, linux-kernel
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 <navid.emamdoost@gmail.com>
> ---
> 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,
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] nfp: abm: fix memory leak in nfp_abm_u32_knode_replace
2019-09-26 4:53 ` Jakub Kicinski
@ 2019-09-27 1:51 ` Navid Emamdoost
2019-09-27 12:12 ` Markus Elfring
` (2 more replies)
2019-09-27 2:26 ` [PATCH v2] net: flow_offload: " Navid Emamdoost
1 sibling, 3 replies; 11+ messages in thread
From: Navid Emamdoost @ 2019-09-27 1:51 UTC (permalink / raw)
To: jakub.kicinski
Cc: emamd001, smccaman, kjlu, Navid Emamdoost, David S. Miller,
Pablo Neira Ayuso, John Hurley, Colin Ian King, oss-drivers,
netdev, linux-kernel
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(-)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] net: flow_offload: fix memory leak in nfp_abm_u32_knode_replace
2019-09-26 4:53 ` Jakub Kicinski
2019-09-27 1:51 ` [PATCH v3] nfp: abm: " Navid Emamdoost
@ 2019-09-27 2:26 ` Navid Emamdoost
1 sibling, 0 replies; 11+ messages in thread
From: Navid Emamdoost @ 2019-09-27 2:26 UTC (permalink / raw)
To: Jakub Kicinski
Cc: emamd001, smccaman, kjlu, David S. Miller, Pablo Neira Ayuso,
John Hurley, Colin Ian King, oss-drivers, netdev, linux-kernel
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 <navid.emamdoost@gmail.com>
> > ---
> > 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,
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] nfp: abm: fix memory leak in nfp_abm_u32_knode_replace
2019-09-27 1:51 ` [PATCH v3] nfp: abm: " Navid Emamdoost
@ 2019-09-27 12:12 ` Markus Elfring
2019-09-27 21:42 ` Jakub Kicinski
2019-09-27 18:56 ` [PATCH v3] " David Miller
2019-12-04 18:39 ` Jakub Kicinski
2 siblings, 1 reply; 11+ messages in thread
From: Markus Elfring @ 2019-09-27 12:12 UTC (permalink / raw)
To: Navid Emamdoost, Jakub Kicinski, netdev, oss-drivers
Cc: Navid Emamdoost, Kangjie Lu, Stephen A McCamant, Colin Ian King,
David S. Miller, John Hurley, Pablo Neira, linux-kernel,
kernel-janitors
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] nfp: abm: fix memory leak in nfp_abm_u32_knode_replace
2019-09-27 1:51 ` [PATCH v3] nfp: abm: " Navid Emamdoost
2019-09-27 12:12 ` Markus Elfring
@ 2019-09-27 18:56 ` David Miller
2019-12-04 18:39 ` Jakub Kicinski
2 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2019-09-27 18:56 UTC (permalink / raw)
To: navid.emamdoost
Cc: jakub.kicinski, emamd001, smccaman, kjlu, pablo, john.hurley,
colin.king, oss-drivers, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] nfp: abm: fix memory leak in nfp_abm_u32_knode_replace
2019-09-27 12:12 ` Markus Elfring
@ 2019-09-27 21:42 ` Jakub Kicinski
2019-09-28 5:55 ` Markus Elfring
0 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2019-09-27 21:42 UTC (permalink / raw)
To: Markus Elfring
Cc: Navid Emamdoost, netdev, oss-drivers, Navid Emamdoost,
Kangjie Lu, Stephen A McCamant, Colin Ian King, David S. Miller,
John Hurley, Pablo Neira, linux-kernel, kernel-janitors
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!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: nfp: abm: fix memory leak in nfp_abm_u32_knode_replace
2019-09-27 21:42 ` Jakub Kicinski
@ 2019-09-28 5:55 ` Markus Elfring
0 siblings, 0 replies; 11+ messages in thread
From: Markus Elfring @ 2019-09-28 5:55 UTC (permalink / raw)
To: Jakub Kicinski, Navid Emamdoost, netdev, oss-drivers
Cc: Navid Emamdoost, Kangjie Lu, Stephen A McCamant, Colin Ian King,
David S. Miller, John Hurley, Pablo Neira, linux-kernel,
kernel-janitors
>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] nfp: abm: fix memory leak in nfp_abm_u32_knode_replace
2019-09-27 1:51 ` [PATCH v3] nfp: abm: " Navid Emamdoost
2019-09-27 12:12 ` Markus Elfring
2019-09-27 18:56 ` [PATCH v3] " David Miller
@ 2019-12-04 18:39 ` Jakub Kicinski
2 siblings, 0 replies; 11+ messages in thread
From: Jakub Kicinski @ 2019-12-04 18:39 UTC (permalink / raw)
To: Navid Emamdoost
Cc: emamd001, smccaman, kjlu, David S. Miller, Pablo Neira Ayuso,
John Hurley, Colin Ian King, oss-drivers, netdev, linux-kernel
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,
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-12-04 18:40 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-25 18:34 [PATCH] net: flow_offload: fix memory leak in nfp_abm_u32_knode_replace Navid Emamdoost
2019-09-26 1:28 ` Jakub Kicinski
2019-09-26 2:22 ` [PATCH v2] " Navid Emamdoost
2019-09-26 4:53 ` Jakub Kicinski
2019-09-27 1:51 ` [PATCH v3] nfp: abm: " Navid Emamdoost
2019-09-27 12:12 ` Markus Elfring
2019-09-27 21:42 ` Jakub Kicinski
2019-09-28 5:55 ` Markus Elfring
2019-09-27 18:56 ` [PATCH v3] " David Miller
2019-12-04 18:39 ` Jakub Kicinski
2019-09-27 2:26 ` [PATCH v2] net: flow_offload: " Navid Emamdoost
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).