Fix NULL pointer dereference in dsl_deadlist_close() after error

[ryao]

Motivation and Context

c33a55b allowed dsl_deadlist_open() to return errors. However, when dsl_deadlist_open() returns an error, dsl_deadlist_close() is called. If the error is from dmu_bonus_hold(), dsl_deadlist_close() will call dmu_buf_rele() on a NULL pointer.

Closes #17809

Description

We check for a NULL pointer and refrain from calling dmu_buf_rele() if we have one. This should fix the issue, since the pointer should be zero-initialized and remain that way unless a valid address is assigned to it.

Grok 4.5 Build Beta found this bug.

How Has This Been Tested?

The buildbot can test.

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)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair 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.

[amotin]

I don't strongly object it this way, but I think it would be cleaner if dsl_deadlist_open() itself also done some cleanup on error, so that dsl_deadlist_is_open() could return false and dsl_deadlist_close() would not be called on something half-initialized.

[ryao]

I don't strongly object it this way, but I think it would be cleaner if dsl_deadlist_open() itself also done some cleanup on error, so that dsl_deadlist_is_open() could return false and dsl_deadlist_close() would not be called on something half-initialized.

@amotin That would require making code changes in a number of places, because the way that the codebase is written calls dsl_deadlist_close() whenever an error from dsl_deadlist_open() occurs. That said, changing it to be consistent with other open()/close() functions where close() is only called if open() succeeds makes sense. I will revise this patch to do that.

Note that bpobj_open()/bpobj_close() behaves similarly to dsl_deadlist_open()/dsl_deadlist_close(), and has a hack to handle this case. If we are going to modify how this pair works, we should also modify that pair too, especially since this one calls that under the right circumstances. Any objections to rewriting those too?

[amotin]

That would require making code changes in a number of places, because the way that the codebase is written calls dsl_deadlist_close() whenever an error from dsl_deadlist_open() occurs. That said, fixing it to be consistent with other open()/close() functions where close() is only called if open() succeeds makes sense. I will revise this patch to do that.

As I see, dsl_deadlist_close() is called if dsl_deadlist_is_open() returns true. With proper cleanup it won't.

Note that bpobj_open()/bpobj_close() behaves similarly to dsl_deadlist_open()/dsl_deadlist_close(), and has a hack to handle this case. If we are going to modify how this pair works, we should also modify that pair too, especially since this one calls that under the right circumstances. Any objections to rewriting those too?

It might be even better there already, since after failed bpobj_open() bpobj_is_open() seems already returning false. Though it could benefit from mutex_destroy(&bpo->bpo_lock); in the error path.