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

["Jubran, Samih" <sameehj@amazon.com>]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, August 19, 2020 5:17 PM
> To: Jubran, Samih <sameehj@amazon.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; Woodhouse, David
> <dwmw@amazon.co.uk>; Machulsky, Zorik <zorik@amazon.com>;
> Matushevsky, Alexander <matua@amazon.com>; Bshara, Saeed
> <saeedb@amazon.com>; Wilson, Matt <msw@amazon.com>; Liguori,
> Anthony <aliguori@amazon.com>; Bshara, Nafea <nafea@amazon.com>;
> Tzalik, Guy <gtzalik@amazon.com>; Belgazal, Netanel
> <netanel@amazon.com>; Saidi, Ali <alisaidi@amazon.com>; Herrenschmidt,
> Benjamin <benh@amazon.com>; Kiyanovski, Arthur
> <akiyano@amazon.com>; Dagan, Noam <ndagan@amazon.com>
> Subject: RE: [EXTERNAL] [PATCH V2 net-next 1/4] net: ena: ethtool: use
> unsigned long for pointer arithmetics
> 
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
> 
> 
> 
> On Wed, Aug 19, 2020 at 01:43:46PM +0000, sameehj@amazon.com wrote:
> > From: Sameeh Jubran <sameehj@amazon.com>
> >
> > unsigned long is the type for doing maths on pointers.
> 
> Maths on pointers is perfectly valid. The real issue here is you have all your
> types mixed up.

The stat_offset field has the bytes from the start of the struct, the math is perfectly valid IMO¸
I have also went for the extra step and tested it using prints.

> 
> > -                     ptr = (u64 *)((uintptr_t)&ring->tx_stats +
> > -                             (uintptr_t)ena_stats->stat_offset);
> > +                     ptr = (u64 *)((unsigned long)&ring->tx_stats +
> > +                             ena_stats->stat_offset);
> 
> struct ena_ring {
> ...
>         union {
>                 struct ena_stats_tx tx_stats;
>                 struct ena_stats_rx rx_stats;
>         };
> 
> struct ena_stats_tx {
>         u64 cnt;
>         u64 bytes;
>         u64 queue_stop;
>         u64 prepare_ctx_err;
>         u64 queue_wakeup;
>         ...
> }
> 
> &ring->tx_stats will give you a struct *ena_stats_tx. Arithmetic on that,
> adding 1 for example, takes you forward a full ena_stats_tx structure. Not
> what you want.
> 
> &ring->tx_stats.cnt however, will give you a u64 *. Adding 1 to that will give
> you bytes, etc.


If I understand you well, the alternative approach you are suggesting is:

ptr = &ring->tx_stats.cnt + ena_stats->stat_offset;

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