Re: [PATCH net-next] netdevsim: add dummy macsec offload

[Sabrina Dubroca <sd@queasysnail.net>]

2023-06-12, 14:38:28 +0200, Simon Horman wrote:
> On Sun, Jun 11, 2023 at 05:45:33PM +0200, Sabrina Dubroca wrote:
> 
> ...
> 
> > diff --git a/drivers/net/netdevsim/macsec.c b/drivers/net/netdevsim/macsec.c
> > new file mode 100644
> > index 000000000000..355ba2f313df
> > --- /dev/null
> 
> ...
> 
> > +static int nsim_macsec_add_secy(struct macsec_context *ctx)
> > +{
> > +	struct netdevsim *ns = netdev_priv(ctx->netdev);
> > +	int idx;
> > +
> > +	if (ns->macsec.nsim_secy_count == NSIM_MACSEC_MAX_SECY_COUNT)
> > +		return -ENOSPC;
> > +
> > +	for (idx = 0; idx < NSIM_MACSEC_MAX_SECY_COUNT; idx++) {
> > +		if (!ns->macsec.nsim_secy[idx].used)
> > +			break;
> > +	}
> > +
> > +	if (idx == NSIM_MACSEC_MAX_SECY_COUNT)
> > +		netdev_err(ctx->netdev, "%s: nsim_secy_count not full but all SecYs used\n",
> > +			   __func__);
> 
> Hi Sabrina,
> 
> It seems that if this condition is met, then ns->macsec.nsim_secy will
> overflow below.

Right, thanks. It should never happen but I'll change that to return
-ENOSPC as well.

> > +
> > +	netdev_dbg(ctx->netdev, "%s: adding new secy with sci %08llx at index %d\n",
> > +		   __func__, be64_to_cpu(ctx->secy->sci), idx);
> > +	ns->macsec.nsim_secy[idx].used = true;
> > +	ns->macsec.nsim_secy[idx].nsim_rxsc_count = 0;
> > +	ns->macsec.nsim_secy[idx].sci = ctx->secy->sci;
> > +	ns->macsec.nsim_secy_count++;
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int nsim_macsec_add_txsa(struct macsec_context *ctx)
> > +{
> > +	struct netdevsim *ns = netdev_priv(ctx->netdev);
> > +	struct nsim_secy *secy;
> > +	int idx;
> > +
> > +	idx = nsim_macsec_find_secy(ns, ctx->secy->sci);
> > +	if (idx < 0) {
> > +		netdev_err(ctx->netdev, "%s: sci %08llx not found in secy table\n",
> > +			   __func__, be64_to_cpu(ctx->secy->sci));
> 
> Sparse seems pretty unhappy about the type of the argement to be64_to_cpu()
> here and elsewhere. I'm unsure what is the best option but one that
> sprang to mind would be conversion helpers, that cast appropriately.
> f.e. sci_to_cpu()

Ok. Since we've never needed that conversion in drivers/net/macsec.c,
I'll drop the helper in here, unless someone objects to that.

> > +		return -ENOENT;
> > +	}
> > +	secy = &ns->macsec.nsim_secy[idx];
> 
> As also reported by the kernel test robot, a W=1 build complains that secy
> is set but unused here and in to other places below.

Yes [facepalm]

Thanks for the review.

> > +
> > +	netdev_dbg(ctx->netdev, "%s: SECY with sci %08llx, AN %u\n",
> > +		   __func__, be64_to_cpu(ctx->secy->sci), ctx->sa.assoc_num);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nsim_macsec_upd_txsa(struct macsec_context *ctx)
> > +{
> > +	struct netdevsim *ns = netdev_priv(ctx->netdev);
> > +	struct nsim_secy *secy;
> > +	int idx;
> > +
> > +	idx = nsim_macsec_find_secy(ns, ctx->secy->sci);
> > +	if (idx < 0) {
> > +		netdev_err(ctx->netdev, "%s: sci %08llx not found in secy table\n",
> > +			   __func__, be64_to_cpu(ctx->secy->sci));
> > +		return -ENOENT;
> > +	}
> > +	secy = &ns->macsec.nsim_secy[idx];
> > +
> > +	netdev_dbg(ctx->netdev, "%s: SECY with sci %08llx, AN %u\n",
> > +		   __func__, be64_to_cpu(ctx->secy->sci), ctx->sa.assoc_num);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nsim_macsec_del_txsa(struct macsec_context *ctx)
> > +{
> > +	struct netdevsim *ns = netdev_priv(ctx->netdev);
> > +	struct nsim_secy *secy;
> > +	int idx;
> > +
> > +	idx = nsim_macsec_find_secy(ns, ctx->secy->sci);
> > +	if (idx < 0) {
> > +		netdev_err(ctx->netdev, "%s: sci %08llx not found in secy table\n",
> > +			   __func__, be64_to_cpu(ctx->secy->sci));
> > +		return -ENOENT;
> > +	}
> > +	secy = &ns->macsec.nsim_secy[idx];
> > +
> > +	netdev_dbg(ctx->netdev, "%s: SECY with sci %08llx, AN %u\n",
> > +		   __func__, be64_to_cpu(ctx->secy->sci), ctx->sa.assoc_num);
> > +
> > +	return 0;
> > +}
> 
> ...

-- 
Sabrina