netdev - Re: [PATCH V2 net-next 1/4] net: ena: ethtool: use unsigned long for pointer arithmetics

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: <20200906094516.2f8ea69c@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Sun, 6 Sep 2020 09:45:16 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Shay Agroskin <shayagr@...zon.com>
Cc:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        "Jubran, Samih" <sameehj@...zon.com>, Andrew Lunn <andrew@...n.ch>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Woodhouse, David" <dwmw@...zon.co.uk>,
        "Machulsky, Zorik" <zorik@...zon.com>,
        "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>,
        "Kiyanovski, Arthur" <akiyano@...zon.com>,
        "Dagan, Noam" <ndagan@...zon.com>
Subject: Re: [PATCH V2 net-next 1/4] net: ena: ethtool: use unsigned long
 for pointer arithmetics

On Sun, 6 Sep 2020 13:47:13 +0300 Shay Agroskin wrote:
> Maciej Fijalkowski <maciej.fijalkowski@...el.com> writes:
> > I don't want to stir up the pot, but do you really need the 
> > offsetof() of
> > each member in the stats struct? Couldn't you piggyback on 
> > assumption that
> > these stats need to be u64 and just walk the struct with 
> > pointer?
> >
> > 	struct ena_ring *ring;
> > 	int offset;
> > 	int i, j;
> > 	u8 *ptr;
> >
> > 	for (i = 0; i < adapter->num_io_queues; i++) {
> > 		/* Tx stats */
> > 		ring = &adapter->tx_ring[i];
> > 		ptr = (u8 *)&ring->tx_stats;
> >
> > 		for (j = 0; j < ENA_STATS_ARRAY_TX; j++) {
> > 			ena_safe_update_stat((u64 *)ptr, 
> > (*data)++, &ring->syncp);
> > 			ptr += sizeof(u64);
> > 		}
> > 	}
> >
> > I find this as a simpler and lighter solution. There might be 
> > issues with
> > code typed in email client, but you get the idea.
> >  
> >> 
> >> of course we need to convert the stat_offset field to be in 8 
> >> bytes resolution instead.
> >> 
> >> This approach has a potential bug hidden in it. If in the 
> >> future
> >> someone decides to expand the "ena_stats_tx" struct and add a 
> >> field preceding cnt,
> >> cnt will no longer be the beginning of the struct, which will 
> >> cause a bug."
> >> 
> >> Therefore, if you have another way to do this, please share 
> >> it. Otherwise I'd
> >> rather leave this code as it is for the sake of robustness.
> >>   
> >> > 
> >> >      Andrew  
> 
> Hi all,
> 
> We tried to implement your suggestion, and found that removing the 
> stat_offset
> field causes problems that are challenging to solve.
> Removing stat_offset introduces a requirement that the statistics 
> in a stat
> strings array (check [1] for example) and stat variables struct 
> (check [2] for
> example) must be in the same order.
> This requirement is prone to future bugs that might be challenging 
> to locate.
> We also tried to unify the array and struct creation by
> using X macros. At the moment this change requires more time and 
> effort by us
> and our customers need this code merged asap.
> 
> [1] https://elixir.bootlin.com/linux/v5.9-
> rc3/source/drivers/net/ethernet/amazon/ena/ena_ethtool.c#L71
> [2] https://elixir.bootlin.com/linux/v5.9-
> rc3/source/drivers/net/ethernet/amazon/ena/ena_netdev.h#L232
> 
> (This message was sent before but didn't seem to get into the 
> mailing list. Apologies if you got it twice)

Divide the offset by 8, cast &ring->tx_stats to u64 * (without
referencing cnt). That should be fine.