Re: [GIT PULL] SafeSetID LSM changes for 5.4
On Mon, Oct 21, 2019 at 08:58:11AM +0200, Ingo Molnar wrote: > > * Paul E. McKenney wrote: > > > --- a/include/linux/rcupdate.h > > +++ b/include/linux/rcupdate.h > > @@ -383,20 +383,22 @@ do { > > \ > > } while (0) > > > > /** > > - * rcu_swap_protected() - swap an RCU and a regular pointer > > - * @rcu_ptr: RCU pointer > > + * rcu_replace() - replace an RCU pointer, returning its old value > > + * @rcu_ptr: RCU pointer, whose old value is returned > > * @ptr: regular pointer > > - * @c: the conditions under which the dereference will take place > > + * @c: the lockdep conditions under which the dereference will take place > > * > > - * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer > > and > > - * @c is the argument that is passed to the rcu_dereference_protected() > > call > > - * used to read that pointer. > > + * Perform a replacement, where @rcu_ptr is an RCU-annotated > > + * pointer and @c is the lockdep argument that is passed to the > > + * rcu_dereference_protected() call used to read that pointer. The old > > + * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr. > > */ > > -#define rcu_swap_protected(rcu_ptr, ptr, c) do { \ > > +#define rcu_replace(rcu_ptr, ptr, c) > > \ > > +({ \ > > typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \ > > rcu_assign_pointer((rcu_ptr), (ptr)); \ > > - (ptr) = __tmp; \ > > -} while (0) > > + __tmp; \ > > +}) > > One small suggestion, would it make sense to name it "rcu_replace_pointer()"? > > This would make it fit into the pointer handling family of RCU functions: > rcu_assign_pointer(), rcu_access_pointer(), RCU_INIT_POINTER() et al? Easy enough to make the change. I will do that tomorrow and test over the following night. > rcu_swap() would also look a bit weird if used in MM code. ;-) How much RCU swap should we configure on this system? About same amount as reader-writer swap! ;-) Thanx, Paul
Re: [GIT PULL] SafeSetID LSM changes for 5.4
* Paul E. McKenney wrote: > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -383,20 +383,22 @@ do { > \ > } while (0) > > /** > - * rcu_swap_protected() - swap an RCU and a regular pointer > - * @rcu_ptr: RCU pointer > + * rcu_replace() - replace an RCU pointer, returning its old value > + * @rcu_ptr: RCU pointer, whose old value is returned > * @ptr: regular pointer > - * @c: the conditions under which the dereference will take place > + * @c: the lockdep conditions under which the dereference will take place > * > - * Perform swap(@rcu_ptr, @ptr) where @rcu_ptr is an RCU-annotated pointer > and > - * @c is the argument that is passed to the rcu_dereference_protected() call > - * used to read that pointer. > + * Perform a replacement, where @rcu_ptr is an RCU-annotated > + * pointer and @c is the lockdep argument that is passed to the > + * rcu_dereference_protected() call used to read that pointer. The old > + * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr. > */ > -#define rcu_swap_protected(rcu_ptr, ptr, c) do { \ > +#define rcu_replace(rcu_ptr, ptr, c) \ > +({ \ > typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \ > rcu_assign_pointer((rcu_ptr), (ptr)); \ > - (ptr) = __tmp; \ > -} while (0) > + __tmp; \ > +}) One small suggestion, would it make sense to name it "rcu_replace_pointer()"? This would make it fit into the pointer handling family of RCU functions: rcu_assign_pointer(), rcu_access_pointer(), RCU_INIT_POINTER() et al? rcu_swap() would also look a bit weird if used in MM code. ;-) Thanks, Ingo
Re: [GIT PULL] SafeSetID LSM changes for 5.4
On Mon, Sep 23, 2019 at 8:31 PM Micah Morton wrote: > >The best way I know of ensuring this is > for me to personally run the SafeSetID selftest (in > tools/testing/selftests/safesetid/) every release, regardless of > whether we make any changes to SafeSetID itself. Does this sound > sufficient or are there more formal guidelines/processes here that I'm > not aware of? I think that would help, but I wopuld also hope that somebody actually runs Chromium / Chrome OS with a modern kernel. Even if *standard* device installs don't end up having recent kernels, I would assume there are people who are testing development setups? Linus
Re: [GIT PULL] SafeSetID LSM changes for 5.4
On Mon, Sep 23, 2019 at 5:45 PM Linus Torvalds wrote: > > On Mon, Sep 23, 2019 at 4:35 PM James Morris > wrote: > > > > My understanding is that SafeSetID is shipping in ChromeOS -- this was > > part of the rationale for merging it. > > Well, if even the developer didn't test it for two months, I don't > think "it's in upstream" makes any sense or difference. > > Linus Yes, SafeSetID is shipping on Chrome OS, although I agree having that bug in 5.3 without anyone noticing is bad. When Jann sent the last round of patches for 5.3 he had tested the code and everything looked good, although I unfortunately neglected to test it again after a tweak to one of the patches, which of course broke stuff when the patches ultimately went in. Even though this is enabled in production for Chrome OS, none of the Chrome OS devices are using version 5.3 yet, so it went unnoticed on Chrome OS so far. In general the fact that a kernel feature is shipping on Chrome OS isn't an up-to-date assurance that the feature works in the most recent Linux release, as it would likely be months (at least) from when a change makes it into the kernel until that kernel release is ever run on a Chrome OS device (right now the most recent kernel we ship on Chrome OS is 4.19, so I've had to backport the SafeSetID stuff). We've found this SafeSetID LSM to be pretty useful on Chrome OS, and more use cases have popped up than we had in mind when writing it, which suggests others would potentially find it useful as well. But I understand for it to be useful to others it needs to be stable and functional on every release. The best way I know of ensuring this is for me to personally run the SafeSetID selftest (in tools/testing/selftests/safesetid/) every release, regardless of whether we make any changes to SafeSetID itself. Does this sound sufficient or are there more formal guidelines/processes here that I'm not aware of?
Re: [GIT PULL] SafeSetID LSM changes for 5.4
On Mon, Sep 23, 2019 at 4:35 PM James Morris wrote: > > My understanding is that SafeSetID is shipping in ChromeOS -- this was > part of the rationale for merging it. Well, if even the developer didn't test it for two months, I don't think "it's in upstream" makes any sense or difference. Linus
Re: [GIT PULL] SafeSetID LSM changes for 5.4
On Mon, Sep 23, 2019 at 4:30 PM Paul E. McKenney wrote: > > I pushed some (untested) commits out to the dev branch of -rcu, the > overall effect of which is shown in the patch below. The series > adds a new rcu_replace() to avoid confusion with swap(), replaces > uses of rcu_swap_protected() with rcu_replace(), and finally removes > rcu_swap_protected(). > > Is this what you had in mind? > > Unless you tell me otherwise, I will assume that this change is important > but not violently urgent. (As in not for the current merge window.) Ack, looks good to me, Linus
Re: [GIT PULL] SafeSetID LSM changes for 5.4
On Mon, 23 Sep 2019, Linus Torvalds wrote: > Should we just remove safesetid again? It's not really maintained, and > it's apparently not used. It was merged in March (with the first > commit in January), and here we are at end of September and this > happens. My understanding is that SafeSetID is shipping in ChromeOS -- this was part of the rationale for merging it. -- James Morris
Re: [GIT PULL] SafeSetID LSM changes for 5.4
On Mon, Sep 23, 2019 at 12:01:49PM -0700, Linus Torvalds wrote: > On Wed, Sep 18, 2019 at 10:41 AM Micah Morton 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? I pushed some (untested) commits out to the dev branch of -rcu, the overall effect of which is shown in the patch below. The series adds a new rcu_replace() to avoid confusion with swap(), replaces uses of rcu_swap_protected() with rcu_replace(), and finally removes rcu_swap_protected(). Is this what you had in mind? Unless you tell me otherwise, I will assume that this change is important but not violently urgent. (As in not for the current merge window.) Thanx, Paul diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c index 46875bb..4c37266 100644 --- a/arch/x86/kvm/pmu.c +++ b/arch/x86/kvm/pmu.c @@ -416,8 +416,8 @@ int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp) *filter = tmp; mutex_lock(>lock); - rcu_swap_protected(kvm->arch.pmu_event_filter, filter, - mutex_is_locked(>lock)); + filter = rcu_replace(kvm->arch.pmu_event_filter, filter, +mutex_is_locked(>lock)); mutex_unlock(>lock); synchronize_srcu_expedited(>srcu); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 0f2c22a..ebb4f15 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1683,7 +1683,7 @@ set_engines(struct i915_gem_context *ctx, i915_gem_context_set_user_engines(ctx); else i915_gem_context_clear_user_engines(ctx); - rcu_swap_protected(ctx->engines, set.engines, 1); + set.engines = rcu_replace(ctx->engines, set.engines, 1); mutex_unlock(>engines_mutex); call_rcu(>rcu, free_engines_rcu); diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index 1f5b5c8..6a38d4a 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -434,8 +434,8 @@ static void scsi_update_vpd_page(struct scsi_device *sdev, u8 page, return; mutex_lock(>inquiry_mutex); - rcu_swap_protected(*sdev_vpd_buf, vpd_buf, - lockdep_is_held(>inquiry_mutex)); + vpd_buf = rcu_replace(*sdev_vpd_buf, vpd_buf, + lockdep_is_held(>inquiry_mutex)); mutex_unlock(>inquiry_mutex); if (vpd_buf) diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 64c96c7..8d17779 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -466,10 +466,10 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work) sdev->request_queue = NULL; mutex_lock(>inquiry_mutex); - rcu_swap_protected(sdev->vpd_pg80, vpd_pg80, - lockdep_is_held(>inquiry_mutex)); - rcu_swap_protected(sdev->vpd_pg83, vpd_pg83, - lockdep_is_held(>inquiry_mutex)); + vpd_pg80 = rcu_replace(sdev->vpd_pg80, vpd_pg80, + lockdep_is_held(>inquiry_mutex)); + vpd_pg83 = rcu_replace(sdev->vpd_pg83, vpd_pg83, + lockdep_is_held(>inquiry_mutex)); mutex_unlock(>inquiry_mutex); if (vpd_pg83) diff --git a/fs/afs/vl_list.c b/fs/afs/vl_list.c index 21eb0c0..e594598 100644 --- a/fs/afs/vl_list.c +++ b/fs/afs/vl_list.c @@ -279,8 +279,8 @@ struct afs_vlserver_list *afs_extract_vlserver_list(struct afs_cell *cell, struct afs_addr_list *old = addrs; write_lock(>lock); -
Re: [GIT PULL] SafeSetID LSM changes for 5.4
On Mon, Sep 23, 2019 at 12:01 PM Linus Torvalds wrote: > > Anyway, this bug would likely had been avoided if rcu_swap_protected() > just returned the old pointer instead of changing the argument. Also, I have to say that the fact that I got the fundamentally buggy commit in a pull request during the 5.3 merge window, and merged it on July 16, but then get the pull request for the fix two months later, after 5.3 has been released, makes me very unhappy with the state of safesetid. The pull request itself was clearly never tested. That's a big problem. And *nobody* used it at all or tested it at all during the whole release process. That's another big problem. Should we just remove safesetid again? It's not really maintained, and it's apparently not used. It was merged in March (with the first commit in January), and here we are at end of September and this happens. So yes, syntactically I'll blame the bad RCU interfaces for why the bug happened. But the fact that the code didn't _work_ and was never tested by anybody for two months, that's not the fault of the RCU code. Linus
Re: [GIT PULL] SafeSetID LSM changes for 5.4
The pull request you sent on Wed, 18 Sep 2019 10:41:06 -0700: > https://github.com/micah-morton/linux.git tags/safesetid-bugfix-5.4 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/1b5fb415442eb3ec946d48afe8c87b0f2fd42d7c Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker
Re: [GIT PULL] SafeSetID LSM changes for 5.4
On Wed, Sep 18, 2019 at 10:41 AM Micah Morton 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
[GIT PULL] SafeSetID LSM changes for 5.4
The following changes since commit 609488bc979f99f805f34e9a32c1e3b71179d10b: Linux 5.3-rc2 (2019-07-28 12:47:02 -0700) are available in the Git repository at: https://github.com/micah-morton/linux.git tags/safesetid-bugfix-5.4 for you to fetch changes up to 21ab8580b383f27b7f59b84ac1699cb26d6c3d69: LSM: SafeSetID: Stop releasing uninitialized ruleset (2019-09-17 11:27:05 -0700) Fix for SafeSetID bug that was introduced in 5.3 Jann Horn sent some patches to fix some bugs in SafeSetID for 5.3. After he had done his testing there were a couple small code tweaks that went in and caused this bug. From what I can see SafeSetID is broken in 5.3 and crashes the kernel every time during initialization if you try to use it. I came across this bug when backporting Jann's changes for 5.3 to older kernels (4.14 and 4.19). I've tested on a Chrome OS device and verified that this change fixes things. Unless I'm missing something it doesn't seem super useful to have this change bake in linux-next, since it is completely broken in 5.3 and nobody noticed. Signed-off-by: Micah Morton Micah Morton (1): LSM: SafeSetID: Stop releasing uninitialized ruleset security/safesetid/securityfs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)