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

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

> -----Original Message-----
> From: Jubran, Samih
> Sent: Thursday, August 20, 2020 3:13 PM
> To: 'Andrew Lunn' <andrew@lunn.ch>
> 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
> 
> 
> 
> > -----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


Ping.