On 23. 7. 2024 5:34, Dave Chinner wrote: > On Mon, Jul 22, 2024 at 03:46:36PM -0400, Paul Moore wrote: >> On Mon, Jul 22, 2024 at 8:30 AM Matus Jokay <matus.jokay@xxxxxxxx> wrote: >>> On 10. 7. 2024 12:40, Mickaël Salaün wrote: >>>> On Tue, Jul 09, 2024 at 10:40:30PM -0400, Paul Moore wrote: >>>>> The LSM framework has an existing inode_free_security() hook which >>>>> is used by LSMs that manage state associated with an inode, but >>>>> due to the use of RCU to protect the inode, special care must be >>>>> taken to ensure that the LSMs do not fully release the inode state >>>>> until it is safe from a RCU perspective. >>>>> >>>>> This patch implements a new inode_free_security_rcu() implementation >>>>> hook which is called when it is safe to free the LSM's internal inode >>>>> state. Unfortunately, this new hook does not have access to the inode >>>>> itself as it may already be released, so the existing >>>>> inode_free_security() hook is retained for those LSMs which require >>>>> access to the inode. >>>>> >>>>> Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx> >>>> >>>> I like this new hook. It is definitely safer than the current approach. >>>> >>>> To make it more consistent, I think we should also rename >>>> security_inode_free() to security_inode_put() to highlight the fact that >>>> LSM implementations should not free potential pointers in this blob >>>> because they could still be dereferenced in a path walk. >>>> >>>>> --- >>>>> include/linux/lsm_hook_defs.h | 1 + >>>>> security/integrity/ima/ima.h | 2 +- >>>>> security/integrity/ima/ima_iint.c | 20 ++++++++------------ >>>>> security/integrity/ima/ima_main.c | 2 +- >>>>> security/landlock/fs.c | 9 ++++++--- >>>>> security/security.c | 26 +++++++++++++------------- >>>>> 6 files changed, 30 insertions(+), 30 deletions(-) >> >> ... >> >>> Sorry for the questions, but for several weeks I can't find answers to two things related to this RFC: >>> >>> 1) How does this patch close [1]? >>> As Mickaël pointed in [2], "It looks like security_inode_free() is called two times on the same inode." >>> Indeed, it does not seem from the backtrace that it is a case of race between destroy_inode and inode_permission, >>> i.e. referencing the inode in a VFS path walk while destroying it... >>> Please, can anyone tell me how this situation could have happened? Maybe folks from VFS... I added them to the copy. >> >> The VFS folks can likely provide a better, or perhaps a more correct >> answer, but my understanding is that during the path walk the inode is >> protected by a RCU lock which allows for multiple threads to access >> the inode simultaneously; this could result in some cases where one >> thread is destroying the inode while another is accessing it. > > Shouldn't may_lookup() be checking the inode for (I_NEW | > I_WILLFREE | I_FREE) so that it doesn't access an inode either not > completely initialised or being evicted during the RCU path walk? > All accesses to the VFS inode that don't have explicit reference > counts have to do these checks... > > IIUC, at the may_lookup() point, the RCU pathwalk doesn't have a > fully validate reference count to the dentry or the inode at this > point, so it seems accessing random objects attached to an inode > that can be anywhere in the setup or teardown process isn't at all > safe... > > -Dave. Indeed, but maybe only VFS maintainers can give us the answer to why may_lookup() does not need at some point check for (I_NEW | I_WILL_FREE | I_FREEING). Thanks, mY
