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:   "Keller, Jacob E" <jacob.e.keller@intel.com>
To:     Jakub Kicinski <kuba@kernel.org>,
        Christophe JAILLET <christophe.jaillet@wanadoo.fr>
CC:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: RE: [PATCH net] genetlink: Fix an error handling path in
 ctrl_dumppolicy_start()
Thread-Topic: [PATCH net] genetlink: Fix an error handling path in
 ctrl_dumppolicy_start()
Thread-Index: AQHZDm0xVyKKaih8LUWrRtPKDet/nq5qv3UAgAAC9zA=
Date:   Mon, 12 Dec 2022 21:23:52 +0000
Message-ID: <CO1PR11MB508966F5AC74C7C328122E11D6E29@CO1PR11MB5089.namprd11.prod.outlook.com>
References: <7186dae6d951495f6918c45f8250e6407d71e88f.1670878949.git.christophe.jaillet@wanadoo.fr>
 <20221212131026.120bdd71@kernel.org>
In-Reply-To: <20221212131026.120bdd71@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?GoNWqP9FDVkBVbG6cXiI9p1sXGkiv4Q/m8LxNVD2+NuWROHM7G7L7F3N7VOq?=
 =?us-ascii?Q?9QAgFnorOYtRL7o7KCSqI8xrs3tp2ioA2LVAnR4n+HXSvj6SC3duufb2gWKB?=
 =?us-ascii?Q?C/gvJiH84GRGn0SZsPu4y5wX5c91XYTAz/2/0Ttj2phaEPe9+K2K5qIIJKdc?=
 =?us-ascii?Q?N8XZ7fTAZlhlcweFlAyzNe9JoV52na8Ih5QH7Kpq543y3/DVncjPP1gv9KGT?=
 =?us-ascii?Q?0KZ0C5IxXURjCs0zC8tyUcOTTsQMcEgcAVpjYcNA286mBaFxv5Sq2BvrOEcC?=
 =?us-ascii?Q?bARb3qltLaB7n7ZQV47MPbqpr1vgV/EJc6R25xdNCeAOO5F1QPbCOw9Xon/O?=
 =?us-ascii?Q?6lirmDGqU/siVuXKmsNcjxRfQgAlwAtaHmkxYWBWIu1sruHNa7OQJdWcWsQW?=
 =?us-ascii?Q?o8ajMIxARQVs111AK96johm4UBvd33ETrKoYLBr2sHKZ/MJc8qNKUt5SRIcT?=
 =?us-ascii?Q?Gy674d48lM/Bt4jhW/9+mzBa9+1zo6ikCyVxoqr05kfXamDYPXD3BLDVQgni?=
 =?us-ascii?Q?kEO4eQhFs7HS1YRa1gHwiRilXtXmC4mx+8W7tYDEUMedkxZBQNZHG2R6k50Q?=
 =?us-ascii?Q?L1klDKJILvl/nabpvgzFj8sG/P7SqHjTTAEVH/BjmmzsbooQgGkCDSvA8MBf?=
 =?us-ascii?Q?U+REBVEt9/l0eHjkqt9u1TNOpsvhT7O2qu6ipZaHewlgCcKKQb1OGqQpJp3i?=
 =?us-ascii?Q?t4RxL7ah57MMoj0/z2E/wkDi5Z6feIE2+3HSPAyd6UuiVdcqlnzupQv/lYIY?=
 =?us-ascii?Q?RzZbpt/tbozu1QOR//LKElAXlQWhpt6QiOAY22G6jETt263sewLcZpieAuWu?=
 =?us-ascii?Q?yx9qHC1FKBL3uUNWP6f9YHS2w9gMp0exVmW55jcQp11ZeAu5t70aWV0/aodW?=
 =?us-ascii?Q?LEWhh6M/ZU+0hGzBixW7ftwgmFRaGYb8rOefu398xmdBYS19MTevGA07rfUU?=
 =?us-ascii?Q?lOrtlc0Jxu1+d2g3xMsKB5mqeIfma025d0vdBZC4ff6SlMgJNG/PwT5jSHH4?=
 =?us-ascii?Q?yFZIHYsrbDlKbmi7fwj5kAKaPRSIpv25H4odF9ehnIlTgx7GpuC9qm3GAwdz?=
 =?us-ascii?Q?czKAU2ny4cwA21DEDOZBg9fECd72ueWDQ+V8fky0NWClmxpUjIbKxJVWrg0w?=
 =?us-ascii?Q?ZbSs8ImzkMU3/txHUboIixGki6NzQVbZcYsxkX0uTniQcZ49loFjo/MafHzZ?=
 =?us-ascii?Q?Sll8Ey1zarKGCL50BNpML8XNE2KUpIOrTB21nOvTuSKELfq06E+mFPa0OPyD?=
 =?us-ascii?Q?ZD5U+hsCoAv4y7UjLVXkxJbKsUGbaOZeZK+byAcka6sohQDgmzRUHp1uRIGU?=
 =?us-ascii?Q?ZUX1peRBEFo8od+gTmSVIPtb5PGOyl0hOZv4e5i3HOqZCwDZXsn/AD1s3IzV?=
 =?us-ascii?Q?z4TobZlM7hEZkAWngWZiqPoMk1dTyh8sWe4dyvcOv797YsB67JFLsTRQMEhe?=
 =?us-ascii?Q?62eDauwOazKR/0TvP7A24ekPdAcEXZraOnfr2jXpQjpZCLB2xmfnA4WUqdHp?=
 =?us-ascii?Q?5V0GDx+H0eINtQmqBQnzvcTss2fNEv/E8ifUAYGmUFXq3+yNZcssmIH0Y5HM?=
 =?us-ascii?Q?DBi5Kw84EqsE1EM/aJ8/BQSXnMu11Di7fXBsZLnH?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a659df81-5d48-437b-39ab-08dadc872ad7
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Dec 2022 21:23:52.3164
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: HmNUpHegBVK1PH9yC0J2UYE3vYSmxKhO5AokJRB7bMVTiHl5BHQbl4oi/tw2gkcDFe7c+rWj56RYOiASx6wiG1z0rxEqHknDuUdkBnGuptA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5411
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        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: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, December 12, 2022 1:10 PM
> To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; Keller, Jacob E
> <jacob.e.keller@intel.com>; linux-kernel@vger.kernel.org; kernel-
> janitors@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH net] genetlink: Fix an error handling path in
> ctrl_dumppolicy_start()
>=20
> On Mon, 12 Dec 2022 22:03:06 +0100 Christophe JAILLET wrote:
> > If this memory allocation fails, some resources need to be freed.
> > Add the missing goto to the error handling path.
> >
> > Fixes: b502b3185cd6 ("genetlink: use iterator in the op to policy map d=
umping")
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> > This patch is speculative.
> >
> > This function is a callback and I don't know how the core works and han=
dles
> > such situation, so review with care!
>=20
> It's fine, the function has pretty much two completely separate paths.
> Dump all ops and dump a single op.
> Anything that allocs state before this point is on the single op path,
> while the iterator is only allocated for dump all.
> This should be evident from the return 0; at the end of the
>   if (tb[CTRL_ATTR_OP])
>=20
> > More-over, should this kmalloc() be a kzalloc()?
> > genl_op_iter_init() below does not initialize all fields, be they are m=
aybe
> > set correctly before uses.

I personally prefer using kzalloc even if we know its not necessary, except=
 in cases where performance of the allocation matters. It helps reduce the =
burden of review as one doesn't need to think "was this initialized?" at le=
ast for the problem of leaking kernel internals.

I know there are also some tools like UBSAN and others which might be able =
to detect access to uninitialized memory, but I am not sure if they're capa=
ble enough at present to handle memory returned by kmalloc or not. If they =
are, then there could be advantage in detecting cases where you did fully e=
xpect initialization to be done.

>=20
> It's fine, op_iters are put on the stack without initializing, iter
> init must (and currently does) work without depending on zeroed memory.

The above said, I think the analysis here is correct and that kmalloc is ok=
 here.

Thanks,
Jake
=20