Hi, I've received a report of a 5.3 kernel crashing in nfs4_show_superblock(). I was part way through preparing a patch when I concluded that the problem wasn't as straight forward as I thought. In the crash, the 'struct file *' passed to nfs4_show_superblock() was NULL. This file was acquired from find_any_file(), and every other caller of find_any_file() checks that the returned value is not NULL (though one BUGs if it is NULL - another WARNs). But nfs4_show_open() and nfs4_show_lock() don't. Maybe they should. I didn't double check, but I suspect they don't hold enough locks to ensure that the files don't get removed. Then I noticed that nfs4_show_deleg() accesses fi_deleg_file without checking if it is NULL - Should it take fi_lock and make sure it is not NULL - and get a counted reference? And maybe nfs4_show_layout() has the same problem? I could probably have worked my way through fixing all of these, but then I discovered that these things are now 'struct nfsd_file *' rather than 'struct file *' and that the helpful documentation says: * Note that this object doesn't * hold a reference to the inode by itself, so the nf_inode pointer should * never be dereferenced, only used for comparison. and yet nfs4_show_superblock() contains: struct inode *inode = f->nf_inode; seq_printf(s, "superblock: \"%02x:%02x:%ld\"", MAJOR(inode->i_sb->s_dev), MINOR(inode->i_sb->s_dev), inode->i_ino); do you see my problem? Is this really safe and the doco wrong? (I note that the use of nf_inode in nfsd_file_mark_find_or_create() looks wrong, but is actually safe). Or should we check if nf_file is non-NULL and use that? In short: - We should check find_any_file() return value - correct? - Do we need extra locking to stabilize fi_deleg_file? - ditto for ->ls_file - how can nfs4_show_superblock safely get s_dev and i_ino from a nfsd_file? Thanks, NeilBrown
Attachment:
signature.asc
Description: PGP signature
