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

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

[Ganesh Babu]

From a91f11fe060729d0009a3271e3a92cead88e2656 Mon Sep 17 00:00:00 2001
From: "Ganesh Babu" <ganesh.babu@ekinops.com>
Date: Wed, 15 Mar 2023 15:01:39 +0530
Subject: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32

Increase mif6c_pifi field in mif6ctl struct
from 16 to 32 bits to support 32-bit ifindices.
The field stores the physical interface (ifindex) for a multicast group.
Passing a 32-bit ifindex via MRT6_ADD_MIF socket option
from user space can cause unpredictable behavior in PIM6.
Changing mif6c_pifi to __u32 allows kernel to handle
32-bit ifindex values without issues.

---
 include/uapi/linux/mroute6.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/mroute6.h b/include/uapi/linux/mroute6.h
index 1d90c21a6251..90e6e771beab 100644
--- a/include/uapi/linux/mroute6.h
+++ b/include/uapi/linux/mroute6.h
@@ -75,7 +75,7 @@ struct mif6ctl {
        mifi_t  mif6c_mifi;             /* Index of MIF */
        unsigned char mif6c_flags;      /* MIFF_ flags */
        unsigned char vifc_threshold;   /* ttl limit */
-       __u16    mif6c_pifi;            /* the index of the physical IF */
+       __u32    mif6c_pifi;            /* the index of the physical IF */
        unsigned int vifc_rate_limit;   /* Rate limiter values (NI) */
 };

--
2.11.0

Signed-off-by: Ganesh Babu <ganesh.babu@ekinops.com>
---

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

[Jakub Kicinski]

On Tue, 28 Mar 2023 07:13:03 +0000 Ganesh Babu wrote:
> From a91f11fe060729d0009a3271e3a92cead88e2656 Mon Sep 17 00:00:00 2001
> From: "Ganesh Babu" <ganesh.babu@ekinops.com>
> Date: Wed, 15 Mar 2023 15:01:39 +0530
> Subject: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32
> 
> Increase mif6c_pifi field in mif6ctl struct
> from 16 to 32 bits to support 32-bit ifindices.
> The field stores the physical interface (ifindex) for a multicast group.
> Passing a 32-bit ifindex via MRT6_ADD_MIF socket option
> from user space can cause unpredictable behavior in PIM6.
> Changing mif6c_pifi to __u32 allows kernel to handle
> 32-bit ifindex values without issues.

The patch is not formatted correctly.
Maybe try git send-email next time?

> diff --git a/include/uapi/linux/mroute6.h b/include/uapi/linux/mroute6.h
> index 1d90c21a6251..90e6e771beab 100644
> --- a/include/uapi/linux/mroute6.h
> +++ b/include/uapi/linux/mroute6.h
> @@ -75,7 +75,7 @@ struct mif6ctl {
>         mifi_t  mif6c_mifi;             /* Index of MIF */
>         unsigned char mif6c_flags;      /* MIFF_ flags */
>         unsigned char vifc_threshold;   /* ttl limit */
> -       __u16    mif6c_pifi;            /* the index of the physical IF */
> +       __u32    mif6c_pifi;            /* the index of the physical IF */

Unfortunately we can't do this. The structure is part of uAPI,
we can't change it's geometry. The kernel must maintain binary
backward compatibility. 

>         unsigned int vifc_rate_limit;   /* Rate limiter values (NI) */
>  };
> 
> --
> 2.11.0
> 
> Signed-off-by: Ganesh Babu <ganesh.babu@ekinops.com>
> ---

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

[Ganesh Babu]

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.

Signed-off-by: Ganesh Babu <ganesh.babu@ekinops.com>

From: Jakub Kicinski <kuba@kernel.org>
Sent: 29 March 2023 07:44
To: Ganesh Babu <ganesh.babu@ekinops.com>
Cc: netdev@vger.kernel.org <netdev@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32 
 
On Tue, 28 Mar 2023 07:13:03 +0000 Ganesh Babu wrote:
> From a91f11fe060729d0009a3271e3a92cead88e2656 Mon Sep 17 00:00:00 2001
> From: "Ganesh Babu" <ganesh.babu@ekinops.com>
> Date: Wed, 15 Mar 2023 15:01:39 +0530
> Subject: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32
> 
> Increase mif6c_pifi field in mif6ctl struct
> from 16 to 32 bits to support 32-bit ifindices.
> The field stores the physical interface (ifindex) for a multicast group.
> Passing a 32-bit ifindex via MRT6_ADD_MIF socket option
> from user space can cause unpredictable behavior in PIM6.
> Changing mif6c_pifi to __u32 allows kernel to handle
> 32-bit ifindex values without issues.

The patch is not formatted correctly.
Maybe try git send-email next time?

> diff --git a/include/uapi/linux/mroute6.h b/include/uapi/linux/mroute6.h
> index 1d90c21a6251..90e6e771beab 100644
> --- a/include/uapi/linux/mroute6.h
> +++ b/include/uapi/linux/mroute6.h
> @@ -75,7 +75,7 @@ struct mif6ctl {
>         mifi_t  mif6c_mifi;             /* Index of MIF */
>         unsigned char mif6c_flags;      /* MIFF_ flags */
>         unsigned char vifc_threshold;   /* ttl limit */
> -       __u16    mif6c_pifi;            /* the index of the physical IF */
> +       __u32    mif6c_pifi;            /* the index of the physical IF */

Unfortunately we can't do this. The structure is part of uAPI,
we can't change it's geometry. The kernel must maintain binary
backward compatibility. 

>         unsigned int vifc_rate_limit;   /* Rate limiter values (NI) */
>  };
> 
> --
> 2.11.0
> 
> Signed-off-by: Ganesh Babu <ganesh.babu@ekinops.com>
> ---

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

[Jakub Kicinski]

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?

There's also the problem of old user space possibly not initializing
the hole, and passing in garbage.

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

[Stephen Hemminger]

On Tue, 2 May 2023 08:57:18 -0700
Jakub Kicinski <kuba@kernel.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?
> 
> 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.

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

[Ganesh Babu]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: 02 May 2023 23:28
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: Ganesh Babu <ganesh.babu@ekinops.com>; netdev@vger.kernel.org;
> linux-kernel@vger.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@kernel.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.

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@networkplumber.org>; Jakub Kicinski
> <kuba@kernel.org>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Ganesh Babu
> <ganesh.babu@ekinops.com>
> Subject: RE: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32
> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: 02 May 2023 23:28
> > To: Jakub Kicinski <kuba@kernel.org>
> > Cc: Ganesh Babu <ganesh.babu@ekinops.com>; netdev@vger.kernel.org;
> > linux-kernel@vger.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@kernel.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.

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.
 

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

[Jakub Kicinski]

On Fri, 16 Jun 2023 09:52:44 +0000 Ganesh Babu wrote:
> 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.

You can try reposting with the commit format fixed after 6.5-rc1 is
released. Make sure the commit messages answers _all_ the questions
and concerns raised. Read this before posting:

https://www.kernel.org/doc/html/next/process/maintainer-netdev.html