netdev - RE: [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute

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 for Android: free password hash cracker in your pocket

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

Message-ID: <BY5PR12MB43220E910463577F0D792965DC3F0@BY5PR12MB4322.namprd12.prod.outlook.com>
Date:   Fri, 18 Sep 2020 04:18:15 +0000
From:   Parav Pandit <parav@...dia.com>
To:     David Ahern <dsahern@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Jiri Pirko <jiri@...dia.com>
Subject: RE: [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour
 and port attribute


> From: David Ahern <dsahern@...il.com>
> Sent: Friday, September 18, 2020 1:32 AM
> 
> On 9/17/20 11:20 AM, Parav Pandit wrote:
> > diff --git a/include/net/devlink.h b/include/net/devlink.h index
> > 48b1c1ef1ebd..1edb558125b0 100644
> > --- a/include/net/devlink.h
> > +++ b/include/net/devlink.h
> > @@ -83,6 +83,20 @@ struct devlink_port_pci_vf_attrs {
> >  	u8 external:1;
> >  };
> >
> > +/**
> > + * struct devlink_port_pci_sf_attrs - devlink port's PCI SF
> > +attributes
> > + * @controller: Associated controller number
> > + * @pf: Associated PCI PF number for this port.
> > + * @sf: Associated PCI SF for of the PCI PF for this port.
> > + * @external: when set, indicates if a port is for an external
> > +controller  */ struct devlink_port_pci_sf_attrs {
> > +	u32 controller;
> > +	u16 pf;
> > +	u32 sf;
> 
> Why a u32? Do you expect to support that many SFs? Seems like even a u16 is
> more than you can adequately name within an IFNAMESZ buffer.
>
I think u16 is likely enough, which let use creates 64K SF ports which is a lot. :-)
Was little concerned that it shouldn't fall short in few years. So picked u32. 
Users will be able to make use of alternative names so just because IFNAMESZ is 16 characters, do not want to limit sfnum size.
What do you think?

> 
> > +	u8 external:1;
> > +};
> > +
> >  /**
> >   * struct devlink_port_attrs - devlink port object
> >   * @flavour: flavour of the port
> 
> 
> > diff --git a/net/core/devlink.c b/net/core/devlink.c index
> > e5b71f3c2d4d..fada660fd515 100644
> > --- a/net/core/devlink.c
> > +++ b/net/core/devlink.c
> > @@ -7855,6 +7889,9 @@ static int
> __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
> >  		n = snprintf(name, len, "pf%uvf%u",
> >  			     attrs->pci_vf.pf, attrs->pci_vf.vf);
> >  		break;
> > +	case DEVLINK_PORT_FLAVOUR_PCI_SF:
> > +		n = snprintf(name, len, "pf%usf%u", attrs->pci_sf.pf, attrs-
> >pci_sf.sf);
> > +		break;
> >  	}
> >
> >  	if (n >= len)
> >
> 
> And as I noted before, this function continues to grow device names and it is
> going to spill over the IFNAMESZ buffer and EINVAL is going to be confusing. It
> really needs better error handling back to users (not kernel buffers).
Alternative names [1] should help to overcome the limitation of IFNAMESZ.
For error code EINVAL, should it be ENOSPC?
If so, should I insert a pre-patch in this series?

[1] ip link property add dev DEVICE [ altname NAME .. ]