Re: [PATCH 1/2] xfrm: interface with if_id 0 should return error

[Antony Antony <antony.antony@secunet.com>]

On Fri, Dec 10, 2021 at 19:22:35 +0200, Eyal Birger wrote:
> On Thu, Dec 9, 2021 at 5:36 PM Antony Antony <antony.antony@secunet.com> wrote:
> >
> > xfrm interface if_id = 0 would cause xfrm policy lookup errors since
> > commit 9f8550e4bd9d ("xfrm: fix disable_xfrm sysctl when used on xfrm interfaces")
> >
> > Now fail to create an xfrm interface when if_id = 0
> >
> > With this commit:
> >  ip link add ipsec0  type xfrm dev lo  if_id 0
> >  Error: if_id must be non zero.
> >
> > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > ---
> >  net/xfrm/xfrm_interface.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
> > index 41de46b5ffa9..57448fc519fc 100644
> > --- a/net/xfrm/xfrm_interface.c
> > +++ b/net/xfrm/xfrm_interface.c
> > @@ -637,11 +637,16 @@ static int xfrmi_newlink(struct net *src_net, struct net_device *dev,
> >                         struct netlink_ext_ack *extack)
> >  {
> >         struct net *net = dev_net(dev);
> > -       struct xfrm_if_parms p;
> > +       struct xfrm_if_parms p = {};
> >         struct xfrm_if *xi;
> >         int err;
> >
> >         xfrmi_netlink_parms(data, &p);
> > +       if (!p.if_id) {
> > +               NL_SET_ERR_MSG(extack, "if_id must be non zero");
> > +               return -EINVAL;
> > +       }
> > +
> >         xi = xfrmi_locate(net, &p);
> >         if (xi)
> >                 return -EEXIST;
> > @@ -666,7 +671,12 @@ static int xfrmi_changelink(struct net_device *dev, struct nlattr *tb[],
> >  {
> >         struct xfrm_if *xi = netdev_priv(dev);
> >         struct net *net = xi->net;
> > -       struct xfrm_if_parms p;
> > +       struct xfrm_if_parms p = {};
> > +
> > +       if (!p.if_id) {
> > +               NL_SET_ERR_MSG(extack, "if_id must be non zero");
> > +               return -EINVAL;
> > +       }
> >
> >         xfrmi_netlink_parms(data, &p);
> >         xi = xfrmi_locate(net, &p);
> 
> Looks good. Maybe this needs a "Fixes:" tag?

I assumed this patch is not ideal for stable releases!
There is a small chance someone was depending old semi broken behavior? And they would find this patch as surprise? So I preferred not add "Fixes: " tag.

Now I notice 9f8550e4bd9d is already in stable/linux-4.19.y. So I think
Fixes tag would be fine.
I will send out a v2 with "Fixes:" tag and let Steffen choose:)

> Reviewed-by: Eyal Birger <eyal.birger@gmail.com>

thanks Eyal.
-antony