Message-ID: <PAZP264MB4064D9406001EB75D768D0E7FC449@PAZP264MB4064.FRAP264.PROD.OUTLOOK.COM>
Date:   Sat, 27 May 2023 22:34:46 +0000
From:   Ganesh Babu <ganesh.babu@...nops.com>
To:     Stephen Hemminger <stephen@...workplumber.org>,
        Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Ganesh Babu <ganesh.babu@...nops.com>
Subject: RE: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32


> -----Original Message-----
> From: Stephen Hemminger <stephen@...workplumber.org>
> Sent: 02 May 2023 23:28
> To: Jakub Kicinski <kuba@...nel.org>
> Cc: Ganesh Babu <ganesh.babu@...nops.com>; netdev@...r.kernel.org;
> linux-kernel@...r.kernel.org
> Subject: Re: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32
> 
> On Tue, 2 May 2023 08:57:18 -0700
> Jakub Kicinski <kuba@...nel.org> wrote:
> 
> > On Tue, 2 May 2023 08:07:10 +0000 Ganesh Babu wrote:
> > > Thank you for your response. Regarding the proposed change to the
> > > mif6ctl structure in mroute6.h, I would like to clarify, that
> > > changing the datatype of mif6c_pifi from __u16 to __u32 will not
> > > change the offset of the structure members, which means that the
> > > size of the structure remains the same and the ABI remains
> > > compatible. Furthermore, ifindex is treated as an integer in all the
> > > subsystems of the kernel and not as a 16-bit value. Therefore,
> > > changing the datatype of mif6c_pifi from __u16 to __u32 is a natural
> > > and expected change that aligns with the existing practice in the
> > > kernel.
> > > I understand that the mif6ctl structure is part of the uAPI and
> > > changing its geometry is not allowed. However, in this case, we are
> > > not changing the geometry of the structure, as the size of the
> > > structure remains the same and the offset of the structure members
> > > will not change. Thus, the proposed change will not affect the ABI
> > > or the user API. Instead, it will allow the kernel to handle 32-bit
> > > ifindex values without any issues, which is essential for the smooth
> > > functioning of the PIM6 protocol. I hope this explanation clarifies
> > > any concerns you may have had. Let me know if you have any further
> > > questions or need any more details.
> >
> > Please don't top post on the list.
> >
> > How does the hole look on big endian? Does it occupy the low or the
> > high bytes?
> >

We don't need to be concerned about the byte arrangement, whether it
occupies the low or high bytes, in the big-endian machine. The reason
is that the mif6c_pifi variable is only used when calling the
dev_get_by_index() function to retrieve the device information of an
interface. This function expects the interface index to be passed as
an integer. Since both the mif6c_pifi variable and the expected
argument of the dev_get_by_index() function are of the same data type,
there is no possibility of data truncation.

It could have been problematic if mif6c_pifi were 32-bit and the
interface index values passed as arguments to the dev_get_by_index()
function were 16-bit, as this could result in unexpected behavior.
However, the proposed change avoids this issue and ensures
compatibility between the data types, eliminating any concerns about
the byte arrangement in the big-endian machine.

> > There's also the problem of old user space possibly not initializing
> > the hole, and passing in garbage.
> 
> Looks like multicast routing is one of the last places with no netlink API, and
> only ioctl. There is no API to modify multicast routes in iproute2.

In open-source applications like FRR
(https://github.com/FRRouting/frr/blob/master/pimd/pim_mroute.c) and
pim6sd (https://github.com/troglobit/pim6sd.git), 32-bit interface
indices are sometimes assigned to 16-bit variables, potentially causing data
loss. This can result in inaccurate or invalid interface indices being used,
leading to incorrect network interface identification and improper multicast
forwarding. For instance, in FRR's pim_mroute_add_vif function, assigning a
32-bit ifindex to a 16-bit vc_pifi variable truncates the value, risking data
loss. Similarly, in pim6sd's k_add_vif function, a 32-bit uv_ifindex 
assigned to a 16-bit mc.mif6c_pifi variable, also risking data loss if the
value exceeds the 16-bit range.

However, even if the userspace application still maintains the mif6c_pifi
variable as 16-bit, the proposed patch ensures that no issues are created.
This is because the size of the mif6ctl structure remains unchanged, even
after converting the datatype of mif6c_pifi from _u16 to _u32. This guarantees
that there is no risk of data truncation when copying the 16-bit mif6c_pifi
userspace value to the 32-bit mif6c_pifi kernel variable.

Powered by blists - more mailing lists