Linux: always check or verify return of igrab() #11704

adamdmoss · 2021-03-07T20:17:27Z

Motivation and Context

zhold() wraps Linux's igrab(); igrab() and zhold() are used in several places in ZFS where they are expected to succeed, with an eventual symmetric zrele()/iput(). However, if the inode is already in the process of being deleted then igrab() can fail and return NULL; in this case a reference is not created on the inode and it a bug to subsequently iput() it (possible premature recycling of the inode memory leading to hard-to-diagnose issues).

Description

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

behlendorf · 2021-03-08T18:26:28Z

Thanks for improving this bit of the code. In general the original undocumented rational for not checking for NULL when using zhold() is that it was only called when the inode was already known to have a reference. In which case it couldn't fail, because the first existing reference would ensure it was never in the process of being deleted. This is always going to be the case for any Linux VFS callback we register which passes us a dentry or an inode. The main place where this is a concern is in zfs_zget() where we might be getting the first reference, and therefore we do need to check for NULL.

I agree we should really make this all much more clear. A reasonable approach might be to add a VERIFY() and a short comment for the cases where we know there must already be a reference. Then add NULL checks as you've done if there are any places left where this may not be true.

adamdmoss · 2021-03-08T18:56:14Z

I honestly can't tell which cases know there's already a ref vs those which don't; should I assume that all the existing code is correct and just VERIFY() that?

Should I make the same assumption that igrab() is always being used where there's already a ref, as well as zhold()?

behlendorf · 2021-03-08T19:45:00Z

It's definitely not very clear. But I believe it's the case that we're checking the igrab() return value everywhere we should be and it would be safe to wrap them all in a VERIFY(). If that's not the case, this would be a good way to find out!

A good example of a place where we know we must have a reference, and it's not currently checked, would be the igrab() in zpl_link(). This is the zpl_dir_inode_operations->link() callback registered with the Linux VFS and it is passed the old_dentry as an argument from which we get the inode. This dentry must have a reference on the inode it points to so we know the igrab() here cannot fail. This will be the case for any of registered VFS callbacks, most (maybe all?) of which pass us a dentry or an inode which will have a reference held by the VFS caller.

adamdmoss · 2021-03-08T23:06:51Z

Is this version more what you had in mind?

behlendorf

Yeah, think this is preferable since it makes it clear why it can never be NULL.

module/os/linux/zfs/zfs_dir.c

include/os/linux/zfs/sys/zfs_znode_impl.h

module/os/linux/zfs/zfs_ctldir.c

module/os/linux/zfs/zfs_vfsops.c

module/os/linux/zfs/zpl_inode.c

module/zfs/zfs_log.c

adamdmoss · 2021-03-11T00:22:48Z

I think this is done now.

behlendorf · 2021-03-12T01:21:17Z

Functionally this looks good to me, could you just address the style comments about lines over 80 characters. You can run make cstyle locally to check for style issues. I think you could also convert those those igrab()s where you added a verify to just use the updated zhold() wrapper.

module/os/linux/zfs/zfs_ctldir.c

adamdmoss · 2021-03-12T03:01:16Z

Oh... yes... sorry, forgot about checkstyle.

behlendorf

Looks good!

Signed-off-by: Adam Moss <[email protected]>

adamdmoss · 2021-03-16T22:53:04Z

Hi @behlendorf is this waiting on anything more from me? Thanks.

behlendorf · 2021-03-16T23:25:58Z

@adamdmoss no this one was just blocked on me to circle back to merge it. Although more review is always welcome! I'll go ahead and merge it now. Thanks!

zhold() wraps igrab() on Linux, and igrab() may fail when the inode is in the process of being deleted. This means zhold() must only be called when a reference exists and therefore it cannot be deleted. This is the case for all existing consumers so add a VERIFY and a comment explaining this requirement. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Adam Moss <[email protected]> Closes openzfs#11704

zhold() wraps igrab() on Linux, and igrab() may fail when the inode is in the process of being deleted. This means zhold() must only be called when a reference exists and therefore it cannot be deleted. This is the case for all existing consumers so add a VERIFY and a comment explaining this requirement. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Adam Moss <[email protected]> Closes #11704

behlendorf added the Status: Work in Progress Not yet ready for general review label Mar 8, 2021

adamdmoss marked this pull request as ready for review March 8, 2021 23:07

adamdmoss changed the title ~~WIP: always check return of zhold() and igrab(), enforce check of return w/gcc~~ Always check return of zhold() and igrab(), enforce check of return w/gcc Mar 8, 2021

behlendorf reviewed Mar 9, 2021

View reviewed changes

adamdmoss changed the title ~~Always check return of zhold() and igrab(), enforce check of return w/gcc~~ Always check return of zhold() and igrab(), enforce check of retur Mar 9, 2021

adamdmoss changed the title ~~Always check return of zhold() and igrab(), enforce check of retur~~ Always check return of zhold() and igrab(), enforce check of return Mar 9, 2021

adamdmoss changed the title ~~Always check return of zhold() and igrab(), enforce check of return~~ (Linux) - VERIFY() return of igrab() where otherwise unchecked Mar 11, 2021

behlendorf added Status: Code Review Needed Ready for review and testing and removed Status: Work in Progress Not yet ready for general review labels Mar 12, 2021

behlendorf reviewed Mar 12, 2021

View reviewed changes

module/os/linux/zfs/zfs_ctldir.c Show resolved Hide resolved

adamdmoss force-pushed the zgrabcheck branch from ba8f2ce to a474181 Compare March 12, 2021 03:22

behlendorf approved these changes Mar 12, 2021

View reviewed changes

Linux: always check or verify return of igrab()

fb3b7a3

Signed-off-by: Adam Moss <[email protected]>

adamdmoss force-pushed the zgrabcheck branch from a474181 to fb3b7a3 Compare March 12, 2021 04:33

adamdmoss changed the title ~~(Linux) - VERIFY() return of igrab() where otherwise unchecked~~ Linux: always check or verify return of igrab() Mar 12, 2021

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 16, 2021

behlendorf merged commit 1daad98 into openzfs:master Mar 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Linux: always check or verify return of igrab() #11704

Linux: always check or verify return of igrab() #11704

Uh oh!

adamdmoss commented Mar 7, 2021 •

edited

Loading

Uh oh!

behlendorf commented Mar 8, 2021

Uh oh!

adamdmoss commented Mar 8, 2021

Uh oh!

behlendorf commented Mar 8, 2021

Uh oh!

adamdmoss commented Mar 8, 2021

Uh oh!

behlendorf left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamdmoss commented Mar 11, 2021

Uh oh!

behlendorf commented Mar 12, 2021

Uh oh!

Uh oh!

adamdmoss commented Mar 12, 2021

Uh oh!

behlendorf left a comment

Uh oh!

adamdmoss commented Mar 16, 2021

Uh oh!

behlendorf commented Mar 16, 2021

Uh oh!

Uh oh!

Linux: always check or verify return of igrab() #11704

Linux: always check or verify return of igrab() #11704

Uh oh!

Conversation

adamdmoss commented Mar 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

behlendorf commented Mar 8, 2021

Uh oh!

adamdmoss commented Mar 8, 2021

Uh oh!

behlendorf commented Mar 8, 2021

Uh oh!

adamdmoss commented Mar 8, 2021

Uh oh!

behlendorf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamdmoss commented Mar 11, 2021

Uh oh!

behlendorf commented Mar 12, 2021

Uh oh!

Uh oh!

adamdmoss commented Mar 12, 2021

Uh oh!

behlendorf left a comment

Choose a reason for hiding this comment

Uh oh!

adamdmoss commented Mar 16, 2021

Uh oh!

behlendorf commented Mar 16, 2021

Uh oh!

Uh oh!

adamdmoss commented Mar 7, 2021 •

edited

Loading