On Mon, Aug 15, 2022 at 07:02:14PM +0100, Al Viro wrote: > On Mon, Aug 15, 2022 at 10:03:21PM +0800, Jiacheng Xu wrote: > > > Patch: > > Fix this bug by moving the assignment of inode->i_private before > > security_inode_alloc. > > An ad-hoc patch is proposed: > > https://patchwork.kernel.org/project/linux-fsdevel/patch/20211011030956.2459172-1-mudongliangabcd@xxxxxxxxx/ > > ... and that looks like utter bollocks. Why does security_inode_alloc() > look at ->i_private? Which LSM is involved? OK, I see... The role of security_inode_alloc() here is that it's a possible cause of failure. And yes, dealing with that class of bugs here might be better. However, *IF* we go that way, why not move that thing to the very end? Just prior to this_cpu_inc(nr_inodes); Sure, nilfs2 steps only into uninitialized ->i_private. Who's to say that we won't run into somebody deciding that e.g. inode->i_data.private_data is worth a look? Just move the call of security_inode_alloc() down to immediately before the nr_inode increment, with explanation along the lines of "In case of security_inode_alloc() failure the inode is passed to ->destroy_inode(); doing security_inode_alloc() last makes for easier life for ->destroy_inode() instances - they can rely upon the rest of inode_init_always() having been done. It's not just a theoretical problem - nilfs2 has actually stepped into that [reference to report]"
