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>
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/1CAHrh8AA==
Date:   Fri, 16 Jun 2023 09:52:44 +0000
Message-ID: <PAZP264MB406423636D35E139C50BCD2BFC58A@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>
 <PAZP264MB4064D9406001EB75D768D0E7FC449@PAZP264MB4064.FRAP264.PROD.OUTLOOK.COM>
In-Reply-To: <PAZP264MB4064D9406001EB75D768D0E7FC449@PAZP264MB4064.FRAP264.PROD.OUTLOOK.COM>
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?k1qUlHC9sdKmhuo0wugg0XngMQZGVspQeMWVytbrVqFfqU3lEA20AEKsBwc3?=
 =?us-ascii?Q?TxzE/DJwcyQGTIgkzkOFQc7GJ5HIzQWxVHfCSqogGnnc1P6YFdGk+BoC4k63?=
 =?us-ascii?Q?BhcR8blEVOS1DExX0hVcm6uQWkIifgR239l4F7YBEq/HoVhKrBQO/QTluevU?=
 =?us-ascii?Q?Eu/zNRwo2tRGGDjFMtKk8x24DDCldCi3O+e57VBkUykEskaHwmzCoO2Cz4un?=
 =?us-ascii?Q?XDkFnuAaf/INDtu0Eegea0pxofg1o49hTRu94I5ZFDIRioBaMDn3uKPXn/C2?=
 =?us-ascii?Q?wn4QqmtYf9hwg16oGrXNnwe8Ks0csdoTe25Yfv2dMxy1P0WIuzqbNVQUtH2m?=
 =?us-ascii?Q?ZPjiJPMgO+laNZ/J1l0raUFgo5RsP9ZK8JP7vz8/BHplQ0TaJPM3bHqomrqe?=
 =?us-ascii?Q?u2WQPQ7yOmaZKICEZMPP1ppHhf2BDUsJcBnE9BKJdjDN6uuJ7SwQAPP1bhk+?=
 =?us-ascii?Q?/qBDV4oAnkfKHtvn/7It6N/h4TfeT2rwhHeH1+5TLjvRwUoyTMmTXi3okMMU?=
 =?us-ascii?Q?Cx1/kBCBnrQs5caBilbPqfLyIV7zVCqsRCI4zzLrR1r46m5t9I3trJGlSxYX?=
 =?us-ascii?Q?VDWaDZLkVZo/cJHP6wS7rDfPGRcZdrIGVuKp5F7DSpv2GOA0+9KmClyNCTga?=
 =?us-ascii?Q?KxZpSt/BPalK1FhE0pdGCA+O+A602RdK4oJvbEgitffj2/VnMjVJlp4lx4D/?=
 =?us-ascii?Q?fJfpIMOMMkSIFgGMTSjKfvL9PbtTbSu72/e82YMbtWkihstFgFeew2IrtFNq?=
 =?us-ascii?Q?tjxcNTLB0+ivpsMEicbm04VF9oNL61IoGe8TF8goYyVbPimHNE9qYapUUE0J?=
 =?us-ascii?Q?LrxaTu4R9KkOg3ZcSgsWP/IjFbP69Z8Dhsf5J/Fwzeb86B0Ai9LrEElR51Fz?=
 =?us-ascii?Q?29q0lQGemFQ+OcgmCr19S4jfiMfwE4LBA2nhV9GPzJxbrmhLhkG1DX8u+nZ3?=
 =?us-ascii?Q?SeSlW3sxcKgmCC3JhQM1WRbyl0O51CEXgGS4QVU78wlOcl6ACvBFTAtU0Gn0?=
 =?us-ascii?Q?3IMp1wW3h6CDIPGu+9bM4o00/TpBTZZZtdbVAy+tF6HZlteMH0CdIjx4KcXO?=
 =?us-ascii?Q?YsaAn6J8Vux8AzErsDqr0MvhP10PyT/g0Ddp3yEAsEgcRUEJhmmx7MYuvAuf?=
 =?us-ascii?Q?SB3AzND69BaNWxvgoP00mN7w1rzmkRxKzpfhN3mM4DmU0ZwCKCFYVQT7f7VQ?=
 =?us-ascii?Q?PHxfllZ25O94jBiqBFqz4ZDm6nekGgRDXOKFlkozdeQSlI4ip4RteffAQR70?=
 =?us-ascii?Q?CVWE5tomOTzr93vHYotnvD2aqetn3ek/ySZE894Wo8PjPRBZ/s6TQ9GENxtN?=
 =?us-ascii?Q?zukKMTXTN+wleD2Gq6Q05IzcUm4XkGWuXrnjPPGKrC0qgYALpLEaLRqQ5ls/?=
 =?us-ascii?Q?RvBGI371ATSpVGIz3UT3HFMgJT3jnFFopjVPWEZm8YD+0JdUz80x3kjFZmZU?=
 =?us-ascii?Q?oq60tVskd7ONTxLkB0IAl+HkWe8tF+n1HzMyuLI7DNAjWUUPz2Kq9C+0MSX6?=
 =?us-ascii?Q?m9SRzg/ZsFrbNaihDyvcmx5QavrAme1sNM5coLBdrGw0X7+waiSvOSPgFlwB?=
 =?us-ascii?Q?X7FDO48wjHpgizmwdGNTdgnPAC48bZxrDoaDijiU?=
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: 324b0255-50a7-4345-0ad2-08db6e4f6edc
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Jun 2023 09:52:44.3917
 (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: nAfYWzgnWsFFANxWE67p31eDpxcGwZ9B6B5WeFaqIbpqStk/IMp4QrkJ56CwcGJHUwoKMqDjgZkhT/BFw999MQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MR1P264MB3201
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,
        URIBL_BLOCKED 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: 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
>=20
>=20
> > -----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?
> > >
>=20
> 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.
>=20
> It could have been problematic if mif6c_pifi were 32-bit and the interfac=
e
> 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.
>=20
> > > 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 ipro=
ute2.
>=20
> 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 indice=
s are
> sometimes assigned to 16-bit variables, potentially causing data loss. Th=
is 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 ifinde=
x to
> a 16-bit vc_pifi variable truncates the value, risking data loss. Similar=
ly, 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 1=
6-bit
> range.
>=20
> 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-b=
it
> 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.
=20