| Message ID | 4acb805751f2cf5de8d69e9602a88ec39feff9fc.1644532467.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,1/2] nfp: flower: Fix a potential leak in nfp_tunnel_add_shared_mac() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Thu, Feb 10, 2022 at 11:34:52PM +0100, Christophe JAILLET wrote: > ida_simple_get() returns an id between min (0) and max (NFP_MAX_MAC_INDEX) > inclusive. > So NFP_MAX_MAC_INDEX (0xff) is a valid id. > > In order for the error handling path to work correctly, the 'invalid' > value for 'ida_idx' should not be in the 0..NFP_MAX_MAC_INDEX range, > inclusive. > > So set it to -1. > > Fixes: 20cce8865098 ("nfp: flower: enable MAC address sharing for offloadable devs") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Thanks again for finding and fixing this. Acked-by: Simon Horman <simon.horman@corigine.com>
On Thu, 10 Feb 2022 23:34:52 +0100 Christophe JAILLET wrote: > ida_simple_get() returns an id between min (0) and max (NFP_MAX_MAC_INDEX) > inclusive. > So NFP_MAX_MAC_INDEX (0xff) is a valid id. > > In order for the error handling path to work correctly, the 'invalid' > value for 'ida_idx' should not be in the 0..NFP_MAX_MAC_INDEX range, > inclusive. > > So set it to -1. > > Fixes: 20cce8865098 ("nfp: flower: enable MAC address sharing for offloadable devs") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> This patch is a fix and the other one is refactoring. They can't be in the same series because they need to go to different trees. Please repost the former with [PATCH net] and ~one week later the latter with [PATCH net-next].
On Fri, Feb 11, 2022 at 04:53:56PM -0800, Jakub Kicinski wrote: > On Thu, 10 Feb 2022 23:34:52 +0100 Christophe JAILLET wrote: > > ida_simple_get() returns an id between min (0) and max (NFP_MAX_MAC_INDEX) > > inclusive. > > So NFP_MAX_MAC_INDEX (0xff) is a valid id. > > > > In order for the error handling path to work correctly, the 'invalid' > > value for 'ida_idx' should not be in the 0..NFP_MAX_MAC_INDEX range, > > inclusive. > > > > So set it to -1. > > > > Fixes: 20cce8865098 ("nfp: flower: enable MAC address sharing for offloadable devs") > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > This patch is a fix and the other one is refactoring. They can't be > in the same series because they need to go to different trees. Please > repost the former with [PATCH net] and ~one week later the latter with > [PATCH net-next]. Thanks Jakub. Christophe, please let me know if you'd like me to handle reposting the patches as described by Jakub.
Le 17/02/2022 à 08:59, Simon Horman a écrit : > On Fri, Feb 11, 2022 at 04:53:56PM -0800, Jakub Kicinski wrote: >> On Thu, 10 Feb 2022 23:34:52 +0100 Christophe JAILLET wrote: >>> ida_simple_get() returns an id between min (0) and max (NFP_MAX_MAC_INDEX) >>> inclusive. >>> So NFP_MAX_MAC_INDEX (0xff) is a valid id. >>> >>> In order for the error handling path to work correctly, the 'invalid' >>> value for 'ida_idx' should not be in the 0..NFP_MAX_MAC_INDEX range, >>> inclusive. >>> >>> So set it to -1. >>> >>> Fixes: 20cce8865098 ("nfp: flower: enable MAC address sharing for offloadable devs") >>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> >> This patch is a fix and the other one is refactoring. They can't be >> in the same series because they need to go to different trees. Please >> repost the former with [PATCH net] and ~one week later the latter with >> [PATCH net-next]. > > Thanks Jakub. > > Christophe, please let me know if you'd like me to handle reposting > the patches as described by Jakub. > Hi, If you can, it's fine for me. I must admit that what I consider, as an hobbyist, too much bureaucracy is sometimes discouraging. I do understand the need for maintainers to have things the way they need, but, well, maybe sometimes it is too much. In this particular case, maybe patch 1/2 could be applied to net as-is, and 2/2 just dropped because not really useful. (Just the thoughts of a tired man after a long day at work, don't worry, tomorrow, I'll be in a better mood) CJ
On Thu, Feb 17, 2022 at 07:20:30PM +0100, Christophe JAILLET wrote: > Le 17/02/2022 à 08:59, Simon Horman a écrit : > > On Fri, Feb 11, 2022 at 04:53:56PM -0800, Jakub Kicinski wrote: > > > On Thu, 10 Feb 2022 23:34:52 +0100 Christophe JAILLET wrote: > > > > ida_simple_get() returns an id between min (0) and max (NFP_MAX_MAC_INDEX) > > > > inclusive. > > > > So NFP_MAX_MAC_INDEX (0xff) is a valid id. > > > > > > > > In order for the error handling path to work correctly, the 'invalid' > > > > value for 'ida_idx' should not be in the 0..NFP_MAX_MAC_INDEX range, > > > > inclusive. > > > > > > > > So set it to -1. > > > > > > > > Fixes: 20cce8865098 ("nfp: flower: enable MAC address sharing for offloadable devs") > > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > > > > > This patch is a fix and the other one is refactoring. They can't be > > > in the same series because they need to go to different trees. Please > > > repost the former with [PATCH net] and ~one week later the latter with > > > [PATCH net-next]. > > > > Thanks Jakub. > > > > Christophe, please let me know if you'd like me to handle reposting > > the patches as described by Jakub. > > > Hi, > > If you can, it's fine for me. > > I must admit that what I consider, as an hobbyist, too much bureaucracy is > sometimes discouraging. > > I do understand the need for maintainers to have things the way they need, > but, well, maybe sometimes it is too much. > > In this particular case, maybe patch 1/2 could be applied to net as-is, and > 2/2 just dropped because not really useful. > > > (Just the thoughts of a tired man after a long day at work, don't worry, > tomorrow, I'll be in a better mood) Thanks Christophe, I appreciate your frustration and apologise for my part in it. I'll work on getting this short series accepted upstream.
diff --git a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c index cd50db779dda..9244b35e3855 100644 --- a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c +++ b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c @@ -922,8 +922,8 @@ nfp_tunnel_add_shared_mac(struct nfp_app *app, struct net_device *netdev, int port, bool mod) { struct nfp_flower_priv *priv = app->priv; - int ida_idx = NFP_MAX_MAC_INDEX, err; struct nfp_tun_offloaded_mac *entry; + int ida_idx = -1, err; u16 nfp_mac_idx = 0; entry = nfp_tunnel_lookup_offloaded_macs(app, netdev->dev_addr); @@ -997,7 +997,7 @@ nfp_tunnel_add_shared_mac(struct nfp_app *app, struct net_device *netdev, err_free_entry: kfree(entry); err_free_ida: - if (ida_idx != NFP_MAX_MAC_INDEX) + if (ida_idx != -1) ida_simple_remove(&priv->tun.mac_off_ids, ida_idx); return err;
ida_simple_get() returns an id between min (0) and max (NFP_MAX_MAC_INDEX) inclusive. So NFP_MAX_MAC_INDEX (0xff) is a valid id. In order for the error handling path to work correctly, the 'invalid' value for 'ida_idx' should not be in the 0..NFP_MAX_MAC_INDEX range, inclusive. So set it to -1. Fixes: 20cce8865098 ("nfp: flower: enable MAC address sharing for offloadable devs") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)