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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <7b4627d3-2a69-5700-e985-2fe5c56f03cb@gmail.com>
Date:   Thu, 17 Sep 2020 14:01:50 -0600
From:   David Ahern <dsahern@...il.com>
To:     Parav Pandit <parav@...dia.com>, davem@...emloft.net,
        kuba@...nel.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

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.


> +	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).