linux-kernel - Re: [PATCH] ionic: Fix rx queue allocation in 'ionic_lif

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] [day] [month] [year] [list]

Message-ID: <0a90fbcb-430e-d059-cd1c-7960e7ec6806@pensando.io>
Date:   Sun, 26 Jan 2020 10:36:00 -0800
From:   Shannon Nelson <snelson@...sando.io>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        drivers@...sando.io, jakub.kicinski@...ronome.com,
        davem@...emloft.net
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] ionic: Fix rx queue allocation in 'ionic_lif_alloc()'

On 1/26/20 2:25 AM, Christophe JAILLET wrote:
> The 'struct ionic' has a 'nrxqs_per_lif' field. So use it instead of
> using two times the value of 'ntxqs_per_lif'.
>
> Note that with the current implementation, this patch is a no-op because
> both fields are set to the same value in 'ionic_lifs_size()' which
> is called before reaching 'ionic_lif_alloc()'.
>
> However, it is more future-proof.
>
> Fixes: 1a58e196467f ("ionic: Add basic lif support")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
> ---
> Another alternative could be to use 'alloc_etherdev_mq()' if really using
> the same value for both fields is what is expected.
> ---
>   drivers/net/ethernet/pensando/ionic/ionic_lif.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index 60fd14df49d7..96d3b3e993ad 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -1663,7 +1663,7 @@ static struct ionic_lif *ionic_lif_alloc(struct ionic *ionic, unsigned int index
>   	int err;
>   
>   	netdev = alloc_etherdev_mqs(sizeof(*lif),
> -				    ionic->ntxqs_per_lif, ionic->ntxqs_per_lif);
> +				    ionic->ntxqs_per_lif, ionic->nrxqs_per_lif);
>   	if (!netdev) {
>   		dev_err(dev, "Cannot allocate netdev, aborting\n");
>   		return ERR_PTR(-ENOMEM);

NAK

Even though the NIC configuration seems to allow for different values, 
the rest of the driver assumes that the values are equal and things will 
likely break if they aren't.  This is why the ethtool service 
ionic_set_channels() only allows the user to change the Combined value, 
and not the individual rx or tx values.

If you really want to tweak this, a better change might be to use the 
min of ntxqs and nrxqs, and maybe add a warning log message that they 
are configured with different values.

sln