netdev - Re: [net-next PATCH v8 5/8] cn10k-ipsec: Add SA add/del support for outb ipsec crypto offload

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAeCc_=Uox_JkD2WMs1ZqC-XFHfBnHWALEZC6iWhb0BNWB9Uow@mail.gmail.com>
Date: Tue, 3 Sep 2024 16:03:06 +0530
From: Bharat Bhushan <bharatb.linux@...il.com>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Bharat Bhushan <bbhushan2@...vell.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, sgoutham@...vell.com, gakula@...vell.com, 
	sbhatta@...vell.com, hkelam@...vell.com, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, jerinj@...vell.com, 
	lcherian@...vell.com, richardcochran@...il.com
Subject: Re: [net-next PATCH v8 5/8] cn10k-ipsec: Add SA add/del support for
 outb ipsec crypto offload

On Tue, Sep 3, 2024 at 3:08 PM Sabrina Dubroca <sd@...asysnail.net> wrote:
>
> 2024-09-03, 10:29:34 +0530, Bharat Bhushan wrote:
> > +static int cn10k_ipsec_validate_state(struct xfrm_state *x)
> > +{
> > +     struct net_device *netdev = x->xso.dev;
> > +
> > +     if (x->props.aalgo != SADB_AALG_NONE) {
> > +             netdev_err(netdev, "Cannot offload authenticated xfrm states\n");
>
> This should use extack, to return this information directly to the
> application that's creating the invalid config. You can propagate it
> from cn10k_ipsec_add_state down to this function, and then:
>
>     NL_SET_ERR_MSG_MOD(extack, "Cannot offload authenticated xfrm states");

ack, this and other places
>
>
> > +static int cn10k_ipsec_inb_add_state(struct xfrm_state *x)
> > +{
> > +     struct net_device *netdev = x->xso.dev;
> > +
> > +     netdev_err(netdev, "xfrm inbound offload not supported\n");
>
> Here too, extack.
>
> > +     return -EOPNOTSUPP;
> > +}
> > +
> > +static int cn10k_ipsec_outb_add_state(struct xfrm_state *x)
> > +{
> > +     struct net_device *netdev = x->xso.dev;
> > +     struct cn10k_tx_sa_s *sa_entry;
> > +     struct cpt_ctx_info_s *sa_info;
> > +     struct otx2_nic *pf;
> > +     int err;
> > +
> > +     err = cn10k_ipsec_validate_state(x);
> > +     if (err)
> > +             return err;
> > +
> > +     pf = netdev_priv(netdev);
> > +     if (!mutex_trylock(&pf->ipsec.lock)) {
>
> Why not wait until we can take the lock? Failing to offload the state
> because this lock is temporarily busy isn't nice to users.
>
> > +             netdev_err(netdev, "IPSEC device is busy\n");
> > +             return -EBUSY;
> > +     }
> > +
> > +     if (!(pf->flags & OTX2_FLAG_IPSEC_OFFLOAD_ENABLED)) {
> > +             netdev_err(netdev, "IPSEC not enabled/supported on device\n");
>
> You should also use extack in this function.
>
>
> [...]
> > +static void cn10k_ipsec_del_state(struct xfrm_state *x)
> > +{
> > +     struct net_device *netdev = x->xso.dev;
> > +     struct cn10k_tx_sa_s *sa_entry;
> > +     struct cpt_ctx_info_s *sa_info;
> > +     struct otx2_nic *pf;
> > +     int sa_index;
> > +
> > +     if (x->xso.dir == XFRM_DEV_OFFLOAD_IN)
> > +             return;
> > +
> > +     pf = netdev_priv(netdev);
> > +     if (!mutex_trylock(&pf->ipsec.lock)) {
> > +             netdev_err(netdev, "IPSEC device is busy\n");
> > +             return;
>
> If we can't take the lock, we leave the state installed on the device
> and leak some memory? That's not good. I assume we're going to reach
> HW limits if this happens a bunch of times, and then we can't offload
> ipsec at all anymore?
>
> I think it would be better to wait until we can take the lock.

This is atomic context (in_atomic() is true). So we need to call the
trylock variant.

Thanks
-Bharat

>
> --
> Sabrina
>