RE: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32

[Ganesh Babu]

> -----Original Message-----
> From: Ganesh Babu
> Sent: Sunday, May 28, 2023 4:05 AM
> To: Stephen Hemminger <stephen@xxxxxxxxxxxxxxxxxx>; Jakub Kicinski
> <kuba@xxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Ganesh Babu
> <ganesh.babu@xxxxxxxxxxx>
> Subject: RE: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32
> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@xxxxxxxxxxxxxxxxxx>
> > Sent: 02 May 2023 23:28
> > To: Jakub Kicinski <kuba@xxxxxxxxxx>
> > Cc: Ganesh Babu <ganesh.babu@xxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxx> 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.

I would appreciate any feedback or updates on the status of patch.
Thank you for your attention, and I look forward to hearing from you soon.