| Message ID | 20240703025625.1695052-1-nichen@iscas.ac.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] ntp: fix size argument for kcalloc | expand |
On 7/3/24 04:56, Chen Ni wrote: > The size argument to kcalloc should be the size of desired structure, xsk_pools is a double pointer, so not "desired structure" but rather you should talk about an element size. > not the pointer to it. > > Fixes: 6402528b7a0b ("nfp: xsk: add AF_XDP zero-copy Rx and Tx support") even if the the behavior is not changed, the fix should be targeted to net tree > Signed-off-by: Chen Ni <nichen@iscas.ac.cn> > --- > drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c > index 182ba0a8b095..768f22cd3d02 100644 > --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c > +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c > @@ -2539,7 +2539,7 @@ nfp_net_alloc(struct pci_dev *pdev, const struct nfp_dev_info *dev_info, > nn->dp.num_r_vecs, num_online_cpus()); > nn->max_r_vecs = nn->dp.num_r_vecs; > > - nn->dp.xsk_pools = kcalloc(nn->max_r_vecs, sizeof(nn->dp.xsk_pools), > + nn->dp.xsk_pools = kcalloc(nn->max_r_vecs, sizeof(*nn->dp.xsk_pools), > GFP_KERNEL); > if (!nn->dp.xsk_pools) { > err = -ENOMEM; code change is correct, even if the size is the same Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Wed, Jul 03, 2024 at 10:56:25AM +0800, Chen Ni wrote: > The size argument to kcalloc should be the size of desired structure, > not the pointer to it. > > Fixes: 6402528b7a0b ("nfp: xsk: add AF_XDP zero-copy Rx and Tx support") > Signed-off-by: Chen Ni <nichen@iscas.ac.cn> > --- > drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c > index 182ba0a8b095..768f22cd3d02 100644 > --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c > +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c > @@ -2539,7 +2539,7 @@ nfp_net_alloc(struct pci_dev *pdev, const struct nfp_dev_info *dev_info, > nn->dp.num_r_vecs, num_online_cpus()); > nn->max_r_vecs = nn->dp.num_r_vecs; > > - nn->dp.xsk_pools = kcalloc(nn->max_r_vecs, sizeof(nn->dp.xsk_pools), > + nn->dp.xsk_pools = kcalloc(nn->max_r_vecs, sizeof(*nn->dp.xsk_pools), > GFP_KERNEL); > if (!nn->dp.xsk_pools) { > err = -ENOMEM; > -- Looks good to me, thanks. Signed-off-by: Louis Peens <louis.peens@corigine.com>
On Wed, Jul 03, 2024 at 10:56:25AM +0800, Chen Ni wrote: > The size argument to kcalloc should be the size of desired structure, > not the pointer to it. > > Fixes: 6402528b7a0b ("nfp: xsk: add AF_XDP zero-copy Rx and Tx support") > Signed-off-by: Chen Ni <nichen@iscas.ac.cn> nit: s/ntp/nfp/ in the subject ...
On Wed, 2024-07-03 at 11:16 +0200, Przemek Kitszel wrote: > On 7/3/24 04:56, Chen Ni wrote: > > The size argument to kcalloc should be the size of desired structure, > > xsk_pools is a double pointer, so not "desired structure" but rather you > should talk about an element size. > > > not the pointer to it. > > > > Fixes: 6402528b7a0b ("nfp: xsk: add AF_XDP zero-copy Rx and Tx support") > > even if the the behavior is not changed, the fix should be targeted to > net tree This patch is IMHO more a cleanup than a real fix. As such it's more suited for net-next. For the same reason I think it should not go to stable, so I'm dropping the fixes tag, too. Thanks, Paolo
On Thu, 2024-07-04 at 11:36 +0200, Paolo Abeni wrote: > On Wed, 2024-07-03 at 11:16 +0200, Przemek Kitszel wrote: > > On 7/3/24 04:56, Chen Ni wrote: > > > The size argument to kcalloc should be the size of desired structure, > > > > xsk_pools is a double pointer, so not "desired structure" but rather you > > should talk about an element size. > > > > > not the pointer to it. > > > > > > Fixes: 6402528b7a0b ("nfp: xsk: add AF_XDP zero-copy Rx and Tx support") > > > > even if the the behavior is not changed, the fix should be targeted to > > net tree > > This patch is IMHO more a cleanup than a real fix. As such it's more > suited for net-next. For the same reason I think it should not go to > stable, so I'm dropping the fixes tag, too. Thinking again about it, this patch has a few things to be cleaned-up. @Chen Ni, please submit a new revision, adjusting the subj and commit message as per Przemek and Simon feedback and dropping the fixes tag, still targeting net-next. You can retain the already collected tags. Thanks, Paolo
On 7/4/24 11:41, Paolo Abeni wrote: > On Thu, 2024-07-04 at 11:36 +0200, Paolo Abeni wrote: >> On Wed, 2024-07-03 at 11:16 +0200, Przemek Kitszel wrote: >>> On 7/3/24 04:56, Chen Ni wrote: >>>> The size argument to kcalloc should be the size of desired structure, >>> >>> xsk_pools is a double pointer, so not "desired structure" but rather you >>> should talk about an element size. >>> >>>> not the pointer to it. >>>> >>>> Fixes: 6402528b7a0b ("nfp: xsk: add AF_XDP zero-copy Rx and Tx support") >>> >>> even if the the behavior is not changed, the fix should be targeted to >>> net tree >> >> This patch is IMHO more a cleanup than a real fix. As such it's more >> suited for net-next. For the same reason I think it should not go to >> stable, so I'm dropping the fixes tag, too. I'm fine with targeting it at any of the trees. But I still believe it is a fix, even if a trivial one, and even if code "works" - it's a "wrong" code. Here I received similar feedback in a similar case: https://www.mail-archive.com/intel-wired-lan@osuosl.org/msg03252.html and I changed my mind then. > > Thinking again about it, this patch has a few things to be cleaned-up. > > @Chen Ni, please submit a new revision, adjusting the subj and commit > message as per Przemek and Simon feedback and dropping the fixes tag, > still targeting net-next. > > You can retain the already collected tags. > > Thanks, > > Paolo >
On Thu, 4 Jul 2024 12:17:39 +0200 Przemek Kitszel wrote: > >> This patch is IMHO more a cleanup than a real fix. As such it's more > >> suited for net-next. For the same reason I think it should not go to > >> stable, so I'm dropping the fixes tag, too. > > I'm fine with targeting it at any of the trees. > > But I still believe it is a fix, even if a trivial one, and even if code > "works" - it's a "wrong" code. > > Here I received similar feedback in a similar case: > https://www.mail-archive.com/intel-wired-lan@osuosl.org/msg03252.html > and I changed my mind then. Comments, docs, and the MAINTAINERS file are special. This is actually changing the code, and at present results in the same binary getting generated.
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c index 182ba0a8b095..768f22cd3d02 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c @@ -2539,7 +2539,7 @@ nfp_net_alloc(struct pci_dev *pdev, const struct nfp_dev_info *dev_info, nn->dp.num_r_vecs, num_online_cpus()); nn->max_r_vecs = nn->dp.num_r_vecs; - nn->dp.xsk_pools = kcalloc(nn->max_r_vecs, sizeof(nn->dp.xsk_pools), + nn->dp.xsk_pools = kcalloc(nn->max_r_vecs, sizeof(*nn->dp.xsk_pools), GFP_KERNEL); if (!nn->dp.xsk_pools) { err = -ENOMEM;
The size argument to kcalloc should be the size of desired structure, not the pointer to it. Fixes: 6402528b7a0b ("nfp: xsk: add AF_XDP zero-copy Rx and Tx support") Signed-off-by: Chen Ni <nichen@iscas.ac.cn> --- drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)