| 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: <20250206142629.GQ74886@unreal>
Date: Thu, 6 Feb 2025 16:26:29 +0200
From: Leon Romanovsky <leon@...nel.org>
To: Bharat Bhushan <bharatb.linux@...il.com>
Cc: Steffen Klassert <steffen.klassert@...unet.com>,
Andrew Lunn <andrew+netdev@...n.ch>,
Ayush Sawal <ayush.sawal@...lsio.com>,
Bharat Bhushan <bbhushan2@...vell.com>,
Eric Dumazet <edumazet@...gle.com>,
Geetha sowjanya <gakula@...vell.com>,
hariprasad <hkelam@...vell.com>,
Herbert Xu <herbert@...dor.apana.org.au>,
intel-wired-lan@...ts.osuosl.org, Jakub Kicinski <kuba@...nel.org>,
Jay Vosburgh <jv@...sburgh.net>, Jonathan Corbet <corbet@....net>,
linux-doc@...r.kernel.org, linux-rdma@...r.kernel.org,
Louis Peens <louis.peens@...igine.com>, netdev@...r.kernel.org,
oss-drivers@...igine.com, Paolo Abeni <pabeni@...hat.com>,
Potnuri Bharat Teja <bharat@...lsio.com>,
Przemek Kitszel <przemyslaw.kitszel@...el.com>,
Saeed Mahameed <saeedm@...dia.com>,
Subbaraya Sundeep <sbhatta@...vell.com>,
Sunil Goutham <sgoutham@...vell.com>,
Tariq Toukan <tariqt@...dia.com>,
Tony Nguyen <anthony.l.nguyen@...el.com>,
Ilia Lin <ilia.lin@...nel.org>
Subject: Re: [PATCH ipsec-next 1/5] xfrm: delay initialization of offload
path till its actually requested
On Thu, Feb 06, 2025 at 07:29:13PM +0530, Bharat Bhushan wrote:
> On Thu, Feb 6, 2025 at 2:24 PM Leon Romanovsky <leon@...nel.org> wrote:
> >
> > On Thu, Feb 06, 2025 at 02:16:08PM +0530, Bharat Bhushan wrote:
> > > Hi Leon,
> > >
> > > On Wed, Feb 5, 2025 at 11:50 PM Leon Romanovsky <leon@...nel.org> wrote:
> > > >
> > > > From: Leon Romanovsky <leonro@...dia.com>
> > > >
> > > > XFRM offload path is probed even if offload isn't needed at all. Let's
> > > > make sure that x->type_offload pointer stays NULL for such path to
> > > > reduce ambiguity.
> > > >
> > > > Fixes: 9d389d7f84bb ("xfrm: Add a xfrm type offload.")
> > > > Signed-off-by: Leon Romanovsky <leonro@...dia.com>
> > > > ---
> > > > include/net/xfrm.h | 12 +++++++++++-
> > > > net/xfrm/xfrm_device.c | 14 +++++++++-----
> > > > net/xfrm/xfrm_state.c | 22 +++++++++-------------
> > > > net/xfrm/xfrm_user.c | 2 +-
> > > > 4 files changed, 30 insertions(+), 20 deletions(-)
> >
> > <...>
> >
> > > > + x->type_offload = xfrm_get_type_offload(x->id.proto, x->props.family);
> > > > + if (!x->type_offload) {
> >
> > <...>
> >
> > > > + xfrm_put_type_offload(x->type_offload);
> > > > + x->type_offload = NULL;
> > >
> > > We always set type_offload to NULL. Can we move type_offload = NULL in
> > > xfrm_put_type_offload() ?
> >
> > We can, but it will require change to xfrm_get_type_offload() too,
> > otherwise we will get asymmetrical get/put.
>
> "x->type_offload = NULL" is always set after the put() function. so I
> thought that maybe moving "x->type_offload = NULL" to the put()
> function would simplify.
> Yes, get/put will be asymmetric. Maybe setting "x->type_offload" can
> be done in get/put().
> Anyway it is not a major comment. ignore if this does not simplify.
Thanks, let's wait for other comments. If I need respin the series, I'll
change the functions to the proposed below.
Thanks
>
> Thanks
> -Bharat
>
> >
> > Do you want something like that?
> > int xfrm_get_type_offload(struct xfrm_state *x);
> > void xfrm_put_type_offload(struct xfrm_state *x);
> >
> > Thansk
> >
> > >
> > > Thanks
> > > -Bharat
> > >
> > > > /* User explicitly requested packet offload mode and configured
> > > > * policy in addition to the XFRM state. So be civil to users,
> > > > * and return an error instead of taking fallback path.
> > > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > > > index ad2202fa82f3..568fe8df7741 100644
> > > > --- a/net/xfrm/xfrm_state.c
> > > > +++ b/net/xfrm/xfrm_state.c
> > > > @@ -424,11 +424,12 @@ void xfrm_unregister_type_offload(const struct xfrm_type_offload *type,
> > > > }
> > > > EXPORT_SYMBOL(xfrm_unregister_type_offload);
> > > >
> > > > -static const struct xfrm_type_offload *
> > > > -xfrm_get_type_offload(u8 proto, unsigned short family, bool try_load)
> > > > +const struct xfrm_type_offload *xfrm_get_type_offload(u8 proto,
> > > > + unsigned short family)
> > > > {
> > > > const struct xfrm_type_offload *type = NULL;
> > > > struct xfrm_state_afinfo *afinfo;
> > > > + bool try_load = true;
> > > >
> > > > retry:
> > > > afinfo = xfrm_state_get_afinfo(family);
> > > > @@ -456,11 +457,7 @@ xfrm_get_type_offload(u8 proto, unsigned short family, bool try_load)
> > > >
> > > > return type;
> > > > }
> > > > -
> > > > -static void xfrm_put_type_offload(const struct xfrm_type_offload *type)
> > > > -{
> > > > - module_put(type->owner);
> > > > -}
> > > > +EXPORT_SYMBOL(xfrm_get_type_offload);
> > > >
> > > > static const struct xfrm_mode xfrm4_mode_map[XFRM_MODE_MAX] = {
> > > > [XFRM_MODE_BEET] = {
> > > > @@ -609,8 +606,6 @@ static void ___xfrm_state_destroy(struct xfrm_state *x)
> > > > kfree(x->coaddr);
> > > > kfree(x->replay_esn);
> > > > kfree(x->preplay_esn);
> > > > - if (x->type_offload)
> > > > - xfrm_put_type_offload(x->type_offload);
> > > > if (x->type) {
> > > > x->type->destructor(x);
> > > > xfrm_put_type(x->type);
> > > > @@ -784,6 +779,9 @@ void xfrm_dev_state_free(struct xfrm_state *x)
> > > > struct xfrm_dev_offload *xso = &x->xso;
> > > > struct net_device *dev = READ_ONCE(xso->dev);
> > > >
> > > > + xfrm_put_type_offload(x->type_offload);
> > > > + x->type_offload = NULL;
> > > > +
> > > > if (dev && dev->xfrmdev_ops) {
> > > > spin_lock_bh(&xfrm_state_dev_gc_lock);
> > > > if (!hlist_unhashed(&x->dev_gclist))
> > > > @@ -3122,7 +3120,7 @@ u32 xfrm_state_mtu(struct xfrm_state *x, int mtu)
> > > > }
> > > > EXPORT_SYMBOL_GPL(xfrm_state_mtu);
> > > >
> > > > -int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
> > > > +int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
> > > > struct netlink_ext_ack *extack)
> > > > {
> > > > const struct xfrm_mode *inner_mode;
> > > > @@ -3178,8 +3176,6 @@ int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
> > > > goto error;
> > > > }
> > > >
> > > > - x->type_offload = xfrm_get_type_offload(x->id.proto, family, offload);
> > > > -
> > > > err = x->type->init_state(x, extack);
> > > > if (err)
> > > > goto error;
> > > > @@ -3229,7 +3225,7 @@ int xfrm_init_state(struct xfrm_state *x)
> > > > {
> > > > int err;
> > > >
> > > > - err = __xfrm_init_state(x, true, false, NULL);
> > > > + err = __xfrm_init_state(x, true, NULL);
> > > > if (!err)
> > > > x->km.state = XFRM_STATE_VALID;
> > > >
> > > > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > > > index 08c6d6f0179f..82a768500999 100644
> > > > --- a/net/xfrm/xfrm_user.c
> > > > +++ b/net/xfrm/xfrm_user.c
> > > > @@ -907,7 +907,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
> > > > goto error;
> > > > }
> > > >
> > > > - err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV], extack);
> > > > + err = __xfrm_init_state(x, false, extack);
> > > > if (err)
> > > > goto error;
> > > >
> > > > --
> > > > 2.48.1
> > > >
> > > >
> > >
Powered by blists - more mailing lists