netdev - Re: [PATCH] iproute2: fix type incompatibility in ifstat.c

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: <CAFoKQtwcXPMs1ecABfBhJbgXpRqz2OKp=ir3qHO3XbMR4eWVYQ@mail.gmail.com>
Date: Thu, 8 Feb 2024 13:17:58 -0500
From: Stephen Gallagher <sgallagh@...hat.com>
To: stephen@...workplumber.org
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH] iproute2: fix type incompatibility in ifstat.c

On Tue, Feb 6, 2024 at 10:12 PM Stephen Hemminger
<stephen@...workplumber.org> wrote:
>
> On Tue,  6 Feb 2024 09:22:06 -0500
> Stephen Gallagher <sgallagh@...hat.com> wrote:
>
> > Throughout ifstat.c, ifstat_ent.val is accessed as a long long unsigned
> > type, however it is defined as __u64. This works by coincidence on many
> > systems, however on ppc64le, __u64 is a long unsigned.
> >
> > This patch makes the type definition consistent with all of the places
> > where it is accessed.
> >
> > Signed-off-by: Stephen Gallagher <sgallagh@...hat.com>
> > ---
>
> Why not fix the use of unsigned long long to be __u64 instead?
> That would make more sense.
>

FWIW, that's the first path I tried, but it's accessed in dozens of
places, every one of which treats it as a 'long long unsigned'. That
led me to the belief that the specified type was just wrong (or at
minimum, in contravention of the consumers' expectations).

> Looking at ifstat it has other problems.
> It doesn't check the sizes in the netlink response.
>
> It really should be using 64 bit stat counters, not the legacy 32 bit
> values. (ie IFLA_STATS64). Anyone want to take this on?X

I definitely don't have the subject-matter expertise to do this,
sorry. Given that this issue is blocking our builds in Fedora/RHEL, I
was hoping we could try to solve the acute problem and leave
wider-ranging fixes to a separate effort.

> Also, the cpu_hits offload code is a wart. If it is of interest, it should
> be more than one stat.
>
> One last issue, is that change the size of this structure will break old
> history files.
>

I'm not sure I understand this; it looks to me like this is a private
structure. If it has API impact (as in, it's written directly to
output data meant to be re-read), shouldn't it be in a public header
and/or otherwise denoted as API?