Skip to content

Add CodeQL check to detect cause of issue #12014 #17352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ryao
Copy link
Contributor

@ryao ryao commented May 19, 2025

Motivation and Context

This is a simple check that would detect instances of the cause of issue #12014.

Description

This check is currently limited to checking mismatches that occur in the same stack frame. It does not detect across stack frames.

I had originally intended not to send this in favor of developing a more comprehensive check and sending the patch for that, but @behlendorf asked for it in #17340, so I am opening a PR with what I currently have. Note that I had made an initial attempt at making the more comprehensive check on the weekend, but I am not well versed in the CodeQL language, so it will take a while to implement.

How Has This Been Tested?

I verified it identified the issue on ryao/zfs.

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:

This check is currently limited to checking mismatches that occur in the
same stack frame. It does not detect across stack frames.

Signed-off-by: Richard Yao <[email protected]>
ryao added a commit to ryao/zfs that referenced this pull request May 19, 2025
This was caught when doing a manual check to see if openzfs#17352 needed to be
improved to catch mismatches across stack frames of the kind that were
first found in openzfs#17340.

Signed-off-by: Richard Yao <[email protected]>
ryao added a commit to ryao/zfs that referenced this pull request May 19, 2025
PRs like openzfs#17352 have no applicable checkbox, so let us add one.

Signed-off-by: Richard Yao <[email protected]>
@behlendorf behlendorf self-requested a review May 19, 2025 23:21
behlendorf pushed a commit that referenced this pull request May 20, 2025
This was caught when doing a manual check to see if #17352 needed to be
improved to catch mismatches across stack frames of the kind that were
first found in #17340.

Reviewed-by: George Amanakis <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes #17353
behlendorf pushed a commit that referenced this pull request May 20, 2025
PRs like #17352 have no applicable checkbox, so let us add one.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Rob Norris <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes #17354
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could/should assert that flags passed to the both functions are the same. Or at least this specific flag.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label May 20, 2025
@ryao
Copy link
Contributor Author

ryao commented May 20, 2025

I wonder if we could/should assert that flags passed to the both functions are the same. Or at least this specific flag.

We could make a different check for that. I will keep it in mind as something else to try to check after I have improved this. I am spending a few hours on this every few days, so I do not expect to have a revision of this for at least a week.

That said, this API strikes me as being naturally error prone. Changing it to use a cookie approach like spl_fstrans_mark()/spl_fstrans_unmark() does would give us correctness by design. That is because we would have only 1 release function and then pass the "cookie" structure into it. This would come at the expense of additional stack usage and I am not yet of the opinion that we should do this over adopting custom static analysis checks.

@behlendorf
Copy link
Contributor

@ryao if you're still iterating on this can your mark it as a draft until you're ready. That said, I'm also fine with merging this functional, but less comprehensive, version until you come up with something better.

@ryao ryao marked this pull request as draft May 21, 2025 17:36
@github-actions github-actions bot added Status: Work in Progress Not yet ready for general review and removed Status: Accepted Ready to integrate (reviewed, tested) labels May 21, 2025
robn pushed a commit to robn/zfs that referenced this pull request May 23, 2025
This was caught when doing a manual check to see if openzfs#17352 needed to be
improved to catch mismatches across stack frames of the kind that were
first found in openzfs#17340.

Reviewed-by: George Amanakis <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#17353
(cherry picked from commit 83fa80a)
(cherry picked from commit 07d815f)
robn pushed a commit to robn/zfs that referenced this pull request May 23, 2025
This was caught when doing a manual check to see if openzfs#17352 needed to be
improved to catch mismatches across stack frames of the kind that were
first found in openzfs#17340.

Reviewed-by: George Amanakis <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#17353
(cherry picked from commit 83fa80a)
(cherry picked from commit 07d815f2a06573514f51c8601aa60db6fe1a76ad)
robn pushed a commit to robn/zfs that referenced this pull request May 23, 2025
This was caught when doing a manual check to see if openzfs#17352 needed to be
improved to catch mismatches across stack frames of the kind that were
first found in openzfs#17340.

Reviewed-by: George Amanakis <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#17353
(cherry picked from commit 83fa80a)
robn pushed a commit to robn/zfs that referenced this pull request May 23, 2025
PRs like openzfs#17352 have no applicable checkbox, so let us add one.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Rob Norris <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#17354
(cherry picked from commit 2e5e4bb)
@robn robn mentioned this pull request May 23, 2025
14 tasks
robn pushed a commit to robn/zfs that referenced this pull request May 24, 2025
This was caught when doing a manual check to see if openzfs#17352 needed to be
improved to catch mismatches across stack frames of the kind that were
first found in openzfs#17340.

Reviewed-by: George Amanakis <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#17353
(cherry picked from commit 83fa80a)
robn pushed a commit to robn/zfs that referenced this pull request May 24, 2025
PRs like openzfs#17352 have no applicable checkbox, so let us add one.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Rob Norris <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#17354
(cherry picked from commit 2e5e4bb)
tonyhutter pushed a commit that referenced this pull request May 27, 2025
This was caught when doing a manual check to see if #17352 needed to be
improved to catch mismatches across stack frames of the kind that were
first found in #17340.

Reviewed-by: George Amanakis <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes #17353
(cherry picked from commit 83fa80a)
(cherry picked from commit 07d815f)
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request May 27, 2025
PRs like openzfs#17352 have no applicable checkbox, so let us add one.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Rob Norris <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#17354
tonyhutter pushed a commit that referenced this pull request May 28, 2025
This was caught when doing a manual check to see if #17352 needed to be
improved to catch mismatches across stack frames of the kind that were
first found in #17340.

Reviewed-by: George Amanakis <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes #17353
(cherry picked from commit 83fa80a)
tonyhutter pushed a commit that referenced this pull request May 28, 2025
PRs like #17352 have no applicable checkbox, so let us add one.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Rob Norris <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes #17354
(cherry picked from commit 2e5e4bb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Work in Progress Not yet ready for general review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants