On Wed, Sep 18, 2019 at 10:41 AM Micah Morton <mort...@chromium.org> wrote: > > Fix for SafeSetID bug that was introduced in 5.3
So this seems to be a good fix, but the bug itself came from the fact that rcu_swap_protected(..) is so hard to read, and I don't see *why* it's so pointlessly hard to read. Yes, we have some macros that change their arguments, but they have a _reason_ to do so (ie they return two different values) and they tend to be very special in other ways too. But rcu_swap_protected() has no reason for it's odd semantics. Looking at that 'handle_policy_update()' function, it's entirely reasonable to think "pol cannot possibly be NULL". When I looked at the fix patch, that was my initial reaction too, and it's probably the reason Jann's commit 03638e62f55f ("LSM: SafeSetID: rewrite userspace API to atomic updates") had that bug to begin with. I don't see the original discussion at all, it's not on Linux-Security-Module for some reason, so I can't tell when/if the NULL pointer test got deleted. Anyway, this bug would likely had been avoided if rcu_swap_protected() just returned the old pointer instead of changing the argument. There are only a handful or users of that macro, maybe this could be fixed? Adding some of the RCU parties to the participants.. Also, the commit message for this fix was a mess, I feel. It says "SafeSetID: Stop releasing uninitialized ruleset", but the ruleset it releases is perfectly initialized. It just might be NULL because it doesn't _exist_. Linus