Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Ganesh Babu <ganesh.babu@ekinops.com>
To:     Stephen Hemminger <stephen@networkplumber.org>,
        Jakub Kicinski <kuba@kernel.org>
CC:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@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
Thread-Topic: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32
Thread-Index: AQHZYUP13XlrMpW17Ue8ZSjM0wOOK68RBfoAgCx+ieeACdaAAIAAIdAAgCd1/1A=
Date:   Sat, 27 May 2023 22:34:46 +0000
Message-ID: <PAZP264MB4064D9406001EB75D768D0E7FC449@PAZP264MB4064.FRAP264.PROD.OUTLOOK.COM>
References: <PAZP264MB4064279CBAB0D7672726F4A1FC889@PAZP264MB4064.FRAP264.PROD.OUTLOOK.COM>
        <20230328191456.43d2222e@kernel.org>
        <PAZP264MB406414BA18689729DDE24F3DFC659@PAZP264MB4064.FRAP264.PROD.OUTLOOK.COM>
        <20230502085718.0551a86d@kernel.org> <20230502105820.2c27630d@hermes.local>
In-Reply-To: <20230502105820.2c27630d@hermes.local>
Accept-Language: en-IN, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?yaW4ItbjiU+iHeBY9HR+eFySVbYw4BFvU8KDO3Dc/par8Mr3l6yIw/0EfYXT?=
 =?us-ascii?Q?cV8O5Ei0kQVyBQnnHWFDlJ0sboaNBKsTh/n0Oo1gcGFv9XN2q5pHIPEhuMq2?=
 =?us-ascii?Q?k59PgTM3gfj/WN5oERNkI3FnPUVJuU2VNZekpRvhpN5xNDjctHjQjO7NoizU?=
 =?us-ascii?Q?Ty1+VRzwoMAsUlLCMC1x19ggjKw7WUt59WlsRNoAdZ7v6Qhn2hRF7t+HbtEr?=
 =?us-ascii?Q?6m70netzbC3xbnNOMFoEGLWGyMBK090ratbGvWSSt1ewlausUpiHcX0xwL1a?=
 =?us-ascii?Q?9gbLFi2T0o87hBKY7TcgCkExAIJB/U1teWjPwAwGFfw4w5MKHY78H5T8NlK+?=
 =?us-ascii?Q?eqeM22wWpzteLNdAag7eTV2dG8RVsJNp6S1y0tgCOIk/cE9CSiy7o6SvmSbl?=
 =?us-ascii?Q?1q/M6s1NzH2sPy02ucaPh+pj1256EMC4WKgPltFMK+SxZR3QnXaHyf2W27z6?=
 =?us-ascii?Q?ArLtJSgRMIqiqTPe300tf4Co418PNWnof2ATaaqqG2c9l1BwJo2fxfxhwHez?=
 =?us-ascii?Q?e11078Q9XYlhLzfHQs3jg2NlxR249JwpmnklEW9PqA0fs7ShBBl7sxkAtPjq?=
 =?us-ascii?Q?1oNu1sq7vmxS+qI7SdRMK/+/mrbCkH3o4WfFOlZ4VGDW2QWhyQ9yWPxDtU7c?=
 =?us-ascii?Q?nMazGjtarjhW1LCxR+ZFkx2i9eIoP4Os1oHIndVLAXxgGmW+qC3EcwYxAodt?=
 =?us-ascii?Q?KauM0MVFW3gwm2x9TCTgJJCbupvVQWeAe1J9x3WS/O+/szzoXF9Uj3erY7hP?=
 =?us-ascii?Q?ytZC67Zlb4Tt9093vRsNzjpbANj08om4kwSVd/JsGWx34VuKJkB814W5wHqz?=
 =?us-ascii?Q?kesZV3hGt2qDmardjZ7A7iX9gTmunBPDtcAB3sGIrGCvTZ7Ylf4QnhboMoYI?=
 =?us-ascii?Q?TNt5bkd9QiO4bUmem/GFBJeNFb7RkQeuOeEOKCsyPTRH0PaA/nv3E/9LXgES?=
 =?us-ascii?Q?VHN8+4zEjOXvEAxAs2nyfgM5otmtRidTbloE8zROmm+NpYfsEgpTU4UHqQmu?=
 =?us-ascii?Q?1lE1sLJyYe+YOEckxCJnPAHrBro9X8RxBFRQnF8DQ47+h6EZRl2lJm1/zmq6?=
 =?us-ascii?Q?aZ5620Gg2ZrQRsjXlT1/JMsENtond8uFRXBrTjpUqhiyhrJpXl2nV5vSTnBS?=
 =?us-ascii?Q?+CDvgQ6rDfsl71Y1s4pXfabhWGTXCwC6k0iIZkj7Z0EqO5qkRVD5o8uHRfmO?=
 =?us-ascii?Q?T31C2NNBBvmQSCUdRAnp7cUlgi6kU0s39hNbEi2h67Z9DgZBf1kSYVvWXiQG?=
 =?us-ascii?Q?HiLS/s4yl2GAD/l/R7raP4EilT+XBiU7sG6wZQ7xF+C2HAL2tVrQcz4TkV0c?=
 =?us-ascii?Q?5llrj/LlfUTaCrdlJTQKzkTTwFi0O6eonrRoOKCPZxdrsF38xCRLX1bFWtYQ?=
 =?us-ascii?Q?AlR6wIxobbBq05RirbBMDtziw3WYgCwuYyvyXo0qQkZh76I5jPOYF6iuJtfc?=
 =?us-ascii?Q?o3adQ0YrtgD1ELyICw3jnvaNyjwvYut/d19sfNl6L76cyJcm6dLlqMQsiuOp?=
 =?us-ascii?Q?oa+S8Q6BGnyy0P0QmtFHBPVzbZFlzfnBxllXzs/g8JHcFBiEqu8R1xFNQ6ol?=
 =?us-ascii?Q?sCqkGyDOTNOtgXXmVNIsu50LcD5UmMG+yadCqpff?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: ekinops.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PAZP264MB4064.FRAP264.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-Network-Message-Id: 0074e422-ac4a-4aa3-17ea-08db5f0292e9
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 May 2023 22:34:46.2004
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: f57b78a6-c654-4771-a72f-837275f46179
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ZFOjae0F65PN7CaxkwOx5aOFVWF1jZ/l9GvotezSVLu+hR6gjqn6+AJiY3RXpkOrxRJhTMn2ryyec5wYs/CpJQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MR1P264MB2255
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----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
>=20
> On Tue, 2 May 2023 08:57:18 -0700
> Jakub Kicinski <kuba@kernel.org> wrote:
>=20
> > 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.
>=20
> Looks like multicast routing is one of the last places with no netlink AP=
I, 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 dat=
a
loss. This can result in inaccurate or invalid interface indices being used=
,
leading to incorrect network interface identification and improper multicas=
t
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 da=
ta
loss. Similarly, in pim6sd's k_add_vif function, a 32-bit uv_ifindex=20
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 guarant=
ees
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.