| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aWYZiWTX4Q9SkKko@rkannoth-OptiPlex-7090>
Date: Tue, 13 Jan 2026 15:38:09 +0530
From: Ratheesh Kannoth <rkannoth@...vell.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [net-next,v3,01/13] octeontx2-af: npc: cn20k: Index management
On 2026-01-11 at 04:29:27, Jakub Kicinski (kuba@...nel.org) wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least pausible.
>
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> octeontx2-af: npc: cn20k: Index management
>
> This patch adds MCAM index management for CN20K silicon, implementing
> allocation schemes for contiguous and non-contiguous MCAM entry
> allocation across 32 subbanks.
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c
> > index 498968bf4cf5..9360886c88e2 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c
>
> [ ... ]
>
> > +static int
> > +npc_subbank_srch_order_parse_n_fill(struct rvu *rvu, char *options,
> > + int num_subbanks)
> > +{
> > + unsigned long w1 = 0, w2 = 0;
> > + char *p, *t1, *t2;
> > + int (*arr)[2];
> > + int idx, val;
> > + int cnt, ret;
> > +
> > + cnt = 0;
> > +
> > + options[strcspn(options, "\r\n")] = 0;
> > +
> > + arr = kcalloc(num_subbanks, sizeof(*arr), GFP_KERNEL);
> > + if (!arr)
> > + return -ENOMEM;
> > +
> > + while ((p = strsep(&options, " ,")) != NULL) {
> > + if (!*p)
> > + continue;
> > +
> > + t1 = strsep(&p, "=");
> > + t2 = strsep(&p, "");
>
> If the user input does not contain an '=' character, strsep() will return
> NULL for t2 here. The subsequent kstrtoint(t2, ...) call would then
> dereference a NULL pointer. Should there be a check for t2 being NULL
> before using it?
ACK.
>
> > +
> > + if (strlen(t1) < 3) {
> > + dev_err(rvu->dev,
> > + "Token length is less than 3 (%s, %s)\n",
> > + t1, t2);
> > + goto err;
> > + }
>
> [ ... ]
>
> > + ret = kstrtoint(t2, 10, &val);
> > + if (ret) {
> > + dev_err(rvu->dev,
> > + "Could not convert token to integer (%s)\n",
> > + t2);
> > + goto err;
> > + }
> > +
> > + arr[cnt][0] = idx;
> > + arr[cnt][1] = val;
> > +
> > + cnt++;
>
> The loop writes to arr[cnt] before checking whether cnt exceeds
> num_subbanks. If a user provides more tokens than expected, this could
> overflow the arr[] buffer. The bounds check occurs after the loop
> completes, but by then the overflow may have already happened. Would
> it make sense to check cnt < num_subbanks before writing to arr[cnt]?
ACK.
>
> > + }
> > +
> > + if (cnt != num_subbanks) {
> > + dev_err(rvu->dev,
> > + "Could find %u tokens, but exact %u tokens needed\n",
> > + cnt, num_subbanks);
> > + goto err;
> > + }
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> > new file mode 100644
> > index 000000000000..c05c0feec41d
> > --- /dev/null
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
>
> [ ... ]
>
> > +static int __maybe_unused
> > +npc_add_to_pf_maps(struct rvu *rvu, u16 mcam_idx, int pcifunc)
> > +{
> > + int rc, idx;
> > + void *map;
> > +
> > + dev_dbg(rvu->dev,
> > + "%s:%d add2maps mcam_idx(%u) to xa_idx2pf map pcifunc=%#x\n",
> > + __func__, __LINE__, mcam_idx, pcifunc);
> > +
> > + rc = xa_insert(&npc_priv.xa_idx2pf_map, mcam_idx,
> > + xa_mk_value(pcifunc), GFP_KERNEL);
> > +
> > + if (rc) {
> > + map = xa_load(&npc_priv.xa_idx2pf_map, mcam_idx);
> > + dev_err(rvu->dev,
> > + "%s:%d failed to insert mcam_idx(%u) to xa_idx2pf map, existing value=%lu\n",
> > + __func__, __LINE__, mcam_idx, xa_to_value(map));
> > + return -EFAULT;
> > + }
> > +
> > + map = xa_load(&npc_priv.xa_pf_map, pcifunc);
> > + idx = xa_to_value(map);
> > +
> > + rc = xa_insert(&npc_priv.xa_pf2idx_map[idx], mcam_idx,
> > + xa_mk_value(pcifunc), GFP_KERNEL);
> > +
> > + if (rc) {
> > + map = xa_load(&npc_priv.xa_pf2idx_map[idx], mcam_idx);
> > + dev_err(rvu->dev,
> > + "%s:%d failed to insert mcam_idx(%u) to xa_pf2idx_map map, earlier value=%lu idx=%u\n",
> > + __func__, __LINE__, mcam_idx, xa_to_value(map), idx);
> > + return -EFAULT;
> > + }
>
> If the second xa_insert() fails, the entry inserted by the first
> xa_insert() into xa_idx2pf_map is not cleaned up. This leaves the two
> maps in an inconsistent state. Should the first entry be erased when
> the second insert fails?
ACK.
>
> [ ... ]
>
> > +static int npc_priv_init(struct rvu *rvu)
> > +{
> > + struct npc_mcam *mcam = &rvu->hw->mcam;
> > + int blkaddr, num_banks, bank_depth;
> > + int num_subbanks, subbank_depth;
> > + u64 npc_const1, npc_const2 = 0;
> > + struct npc_subbank *sb;
> > + u64 cfg;
> > + int i;
> > +
> > + blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
> > + if (blkaddr < 0) {
> > + dev_err(rvu->dev, "%s:%d NPC block not implemented\n",
> > + __func__, __LINE__);
> > + return -ENODEV;
> > + }
> > +
> > + npc_const1 = rvu_read64(rvu, blkaddr, NPC_AF_CONST1);
> > + if (npc_const1 & BIT_ULL(63))
> > + npc_const2 = rvu_read64(rvu, blkaddr, NPC_AF_CONST2);
> > +
> > + num_banks = mcam->banks;
> > + bank_depth = mcam->banksize;
> > +
> > + num_subbanks = FIELD_GET(GENMASK_ULL(39, 32), npc_const2);
> > + npc_priv.num_subbanks = num_subbanks;
> > +
> > + subbank_depth = bank_depth / num_subbanks;
>
> If npc_const1 does not have bit 63 set, npc_const2 remains 0, and
> num_subbanks will be 0. This would cause a division by zero here.
> Should there be a check that num_subbanks is non-zero before this
> division?
ACK.
Powered by blists - more mailing lists