[LU-2900] Null pointer dereference in ll_fsync (llite/file.c) from mkdir in an NFS mounted Lustre fs

• Status: Resolved
• Project: Lustre
• Affects Version/s: Lustre 2.0.0, Lustre 2.2.0, Lustre 2.3.0, Lustre 2.4.0, Lustre 2.1.1, Lustre 2.1.2, Lustre 2.1.3, Lustre 2.1.4
• Fix Version/s: Lustre 2.4.0
• Type: Bug
• Priority: Major
• Reporter: Patrick Farrell (Inactive)
• Assignee: Bob Glossman (Inactive)
• Resolution: Fixed
• Labels: NFS, patch

Environment

SLES11SP1 (Cray Linux Environment)

• Severity: 3

Description

When a Lustre file system is mounted via NFS and a mkdir operation is attempted, a null pointer dereference occurs in ll_fsync.

With 2.x, Lustre added support for different VFS fsync APIs that do not include a dentry parameter.

To make the logic the same in all cases, the old ll_fsync interface was changed to pull the inode from the f_dentry in the file parameter.

In some cases when using the old ll_fsync interface, the caller does not set the file parameter resulting in a NULL dereference. The fix to this is to restore the old logic in those cases: when a dentry parameter is provided, get the inode from that parameter rather than the file parameter.

Here's the current code in llite/file.c (same throughout 2.x):

#ifdef HAVE_FILE_FSYNC_4ARGS
int ll_fsync(struct file *file, loff_t start, loff_t end, int data)
#elif defined(HAVE_FILE_FSYNC_2ARGS)
int ll_fsync(struct file *file, int data)
#else
int ll_fsync(struct file *file, struct dentry *dentry, int data)
#endif
{
         struct inode *inode = file->f_dentry->d_inode;
         struct ll_inode_info *lli = ll_i2info(inode);

Here's the proposed fix:

/* 
 * When dentry is provided (the 'else' case), *file may be null
 * and dentry must be used directly rather than pulled from *file
 * as is done otherwise.
 */
#ifdef HAVE_FILE_FSYNC_4ARGS
int ll_fsync(struct file *file, loff_t start, loff_t end, int data)
#elif defined(HAVE_FILE_FSYNC_2ARGS)
int ll_fsync(struct file *file, int data)
#else
int ll_fsync(struct file *file, struct dentry *dentry, int data)
#endif
{
#if defined(HAVE_FILE_FSYNC_4ARGS) || defined(HAVE_FILE_FSYNC_2ARGS)
        struct inode *inode = file->f_dentry->d_inode;
#else
        struct inode *inode = dentry->d_inode;
#endif
        struct ll_inode_info *lli = ll_i2info(inode);

The fix has been tested at Cray, both under the general acceptance-small tests and specifically for the NFS issue.

I'll be putting the patch in Gerrit shortly.

Issue Links

• is duplicated by: LU-1334 Oops in ll_fsync on Lustre client / N...

Comments

[Jodi Levi (Inactive)]

Bob,
There is no need for a 2.4 patch unless this is also happening with SLES11SP2.

[Bob Glossman (Inactive)]

I don't think this will happen in SLES11SP2. From the description and the proposed fix, this only occurs when neither HAVE_FILE_FSYNC_4ARGS or HAVE_FILE_FSYNC_2ARGS is #define'd. In SLES11SP2 HAVE_FILE_FSYNC_4ARGS is #define'd so the existing code should work fine.

[Patrick Farrell (Inactive)]

Here's the Gerrit link:
http://review.whamcloud.com/#change,5582

[Patrick Farrell (Inactive)]

Bob correctly commented on the patch in Gerrit that the file* is not null - That's my error here.

It is, in fact, the file->f_dentry pointer that is sometimes null. The file pointer is always present.
The code the changes remains the same, but the comments have been changed to reflect this.

My apologies for the error!

Here is the new patch:
http://review.whamcloud.com/5585

[Bob Glossman (Inactive)]

Just FYI you could have just amended your original commit & pushed it again. It would have shown up as Patch Set 2 in the original gerrit change. Somewhat simpler than abandoning one change and creating a whole new one.

[Patrick Farrell (Inactive)]

I don't see how to close this - or if that's possible for me - but the patch has been accepted by Oleg and is in master, so this should be closed.

Thanks!

[Jodi Levi (Inactive)]

Patch landed to master.