netdev - Re: [PATCH V1 net-next 02/11] net: ena: ethtool: add extra properties retrieval via get_priv

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: <20190531223829.GH15954@unicorn.suse.cz>
Date:   Sat, 1 Jun 2019 00:38:29 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     "Machulsky, Zorik" <zorik@...zon.com>,
        "Jubran, Samih" <sameehj@...zon.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "Kiyanovski, Arthur" <akiyano@...zon.com>,
        "Woodhouse, David" <dwmw@...zon.co.uk>,
        "Matushevsky, Alexander" <matua@...zon.com>,
        "Bshara, Saeed" <saeedb@...zon.com>,
        "Wilson, Matt" <msw@...zon.com>,
        "Liguori, Anthony" <aliguori@...zon.com>,
        "Bshara, Nafea" <nafea@...zon.com>,
        "Tzalik, Guy" <gtzalik@...zon.com>,
        "Belgazal, Netanel" <netanel@...zon.com>,
        "Saidi, Ali" <alisaidi@...zon.com>,
        "Herrenschmidt, Benjamin" <benh@...zon.com>
Subject: Re: [PATCH V1 net-next 02/11] net: ena: ethtool: add extra
 properties retrieval via get_priv_flags

On Fri, May 31, 2019 at 09:57:51PM +0000, Machulsky, Zorik wrote:
>     >  
>     > +int ena_com_extra_properties_strings_init(struct ena_com_dev *ena_dev)
>     > +{
>     > +	struct ena_admin_get_feat_resp resp;
>     > +	struct ena_extra_properties_strings *extra_properties_strings =
>     > +			&ena_dev->extra_properties_strings;
>     > +	u32 rc;
>     > +
>     > +	extra_properties_strings->size = ENA_ADMIN_EXTRA_PROPERTIES_COUNT *
>     > +		ENA_ADMIN_EXTRA_PROPERTIES_STRING_LEN;
>     > +
>     > +	extra_properties_strings->virt_addr =
>     > +		dma_alloc_coherent(ena_dev->dmadev,
>     > +				   extra_properties_strings->size,
>     > +				   &extra_properties_strings->dma_addr,
>     > +				   GFP_KERNEL);
>     
>     Do you need to fetch the private flag names on each ETHTOOL_GSTRING
>     request? I suppose they could change e.g. on a firmware update but then
>     even the count could change which you do not seem to handle. Is there
>     a reason not to fetch the names once on init rather then accessing the
>     device memory each time?
>     
>     My point is that ethtool_ops::get_strings() does not return a value
>     which indicates that it's supposed to be a trivial operation which
>     cannot fail, usually a simple copy within kernel memory.
> 
> ena_com_extra_properties_strings_init() is called in probe() only, and not for every ETHTOOL_GSTRING
> request. For the latter we use ena_get_strings(). And just to make sure we are on the same page, extra_properties_strings->virt_addr 
> points to the host memory and not to the device memory. I believe this should answer your concern.

That's what I misunderstood. Sorry for the noise then.

>     > +static void get_private_flags_strings(struct ena_adapter *adapter, u8 *data)
>     > +{
>     > +	struct ena_com_dev *ena_dev = adapter->ena_dev;
>     > +	u8 *strings = ena_dev->extra_properties_strings.virt_addr;
>     > +	int i;
>     > +
>     > +	if (unlikely(!strings)) {
>     > +		adapter->ena_extra_properties_count = 0;
>     > +		netif_err(adapter, drv, adapter->netdev,
>     > +			  "Failed to allocate extra properties strings\n");
>     > +		return;
>     > +	}
>     
>     This is a bit confusing, IMHO. I'm aware we shouldn't really get here as
>     with strings null, count would be zero and ethtool_get_strings()
>     wouldn't call the ->get_strings() callback. But if we ever do, it makes
>     little sense to complain about failed allocation (which happened once on
>     init) each time userspace makes ETHTOOL_GSTRINGS request for private
>     flags.
> 
> I believe we still want to check validity of the strings pointer to keep the driver resilient, however I agree that 
> the logged message is confusing. Let us rework this commit  

Yes, I didn't question the check, only the error message.

Michal