7.2.11-rc1 review

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Vitaly Prosyak <[email protected]>

commit 1a6023edfbb5673f55fab6348f135fb3b1428ea5 upstream.

The vblank counter callback reads display registers and scanout position without holding reset_domain->sem. During GPU recovery, display commit work can call the callback while reset owns the semaphore for writing. The hardware access then triggers the lockdep_assert_held() in amdgpu_device_skip_hw_access() and sets TAINT_WARN when lockdep is enabled.

Try to acquire the reset semaphore for reading before accessing the hardware, and hold it across the counter and scanout-position reads. If reset is already in progress, return DRM's cached software vblank count instead. This avoids blocking behind reset and avoids accessing display hardware while it is being reset.

The cached count is less accurate than the hardware count, but is used only when reset prevents a safe hardware read. This addresses the reset-time warning from this vblank path; it does not suppress or fix unrelated lockdep warnings.

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |    5 +++++
 1 file changed, 5 insertions(+)

@@ -30,6 +30,7 @@
 #include <drm/amdgpu_drm.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_vblank.h>
 #include "amdgpu_uvd.h"
 #include "amdgpu_vce.h"
 #include "atom.h"

@@ -1631,6 +1632,9 @@ u32 amdgpu_get_vblank_counter_kms(struct
 		DRM_ERROR("Invalid crtc %u\n", pipe);
 		return -EINVAL;
 	}
+	if (!down_read_trylock(&adev->reset_domain->sem))
+		return drm_crtc_vblank_count(crtc);
+
 
 	/* The hw increments its frame counter at start of vsync, not at start
 	 * of vblank, as is required by DRM core vblank counter handling.

@@ -1676,6 +1680,7 @@ u32 amdgpu_get_vblank_counter_kms(struct
 		DRM_DEBUG_VBL("NULL mode info! Returned count may be wrong.\n");
 	}
 
+	up_read(&adev->reset_domain->sem);
 	return count;
 }
 

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Alex Deucher <[email protected]>

commit c7f44ec5c4ec760e74351f1843dcb8322f67cbf6 upstream.

Rather than adding a case of every chip where this is seen just check if the FB is at 0 and the ssid is apple. This should cover all chips which fall into this case.

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

@@ -1089,20 +1089,17 @@ void amdgpu_gmc_init_vga_resv_regions(st
 	case CHIP_RENOIR:
 		adev->mman.keep_stolen_vga_memory = true;
 		break;
-	case CHIP_POLARIS10:
-	case CHIP_POLARIS11:
-	case CHIP_POLARIS12:
-		/* MacBookPros with switchable graphics put VRAM at 0 when
-		 * the iGPU is enabled which results in cursor issues if
-		 * the cursor ends up at 0.  Reserve vram at 0 in that case.
-		 */
-		if (adev->gmc.vram_start == 0)
-			adev->mman.keep_stolen_vga_memory = true;
-		break;
 	default:
 		adev->mman.keep_stolen_vga_memory = false;
 		break;
 	}
+	/* MacBookPros with switchable graphics put VRAM at 0 when
+	 * the iGPU is enabled which results in cursor issues if
+	 * the cursor ends up at 0.  Reserve vram at 0 in that case.
+	 */
+	if ((adev->pdev->subsystem_vendor == PCI_VENDOR_ID_APPLE) &&
+	    (adev->gmc.vram_start == 0))
+		adev->mman.keep_stolen_vga_memory = true;
 
 	if (amdgpu_sriov_vf(adev) ||
 	    !amdgpu_device_has_display_hardware(adev)) {

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Mikhail Gavrilov <[email protected]>

commit 16c64495859f13f75602f439588883f2a4a4d6a6 upstream.

Reading or writing a register through the amdgpu_regs2 debugfs file with GRBM bank selection, which umr does for every banked access (a single "umr -b x x x -r <reg>" is enough, as is the wave dump with "-O halt_waves"), makes lockdep report a circular locking dependency, after which lockdep stays off until reboot:

WARNING: possible circular locking dependency detected
umr/63306 is trying to acquire lock:
ffff8882cf4b8350 (&mm->mmap_lock){++++}-{4:4}, at: __might_fault+0x9b/0x140
but task is already holding lock:
ffff8881aec010d0 (&amdgpu_grbm_lock_key){+.+.}-{4:4}, at: amdgpu_debugfs_regs2_op.isra.0+0x20d/0xd00 [amdgpu]
...

  Chain exists of:
    &mm->mmap_lock --> &amdgpu_srbm_lock_key --> &amdgpu_grbm_lock_key
  ...
   __might_fault+0xc5/0x140
   amdgpu_debugfs_regs2_op.isra.0+0x441/0xd00 [amdgpu]
   full_proxy_write+0x10e/0x1d0
   vfs_write+0x1d0/0xf50
   ksys_write+0x116/0x250

amdgpu_debugfs_regs2_op() and amdgpu_debugfs_process_reg_op() call get_user()/put_user() for every dword while holding grbm_idx_mutex or srbm_mutex, and pm.mutex if requested, so a fault there takes mmap_lock inside those locks. The lock hierarchy in amdgpu_lockdep.c puts srbm_mutex and grbm_idx_mutex below notifier_lock, which amdgpu_cs_submit() takes with the BO reservations held, and dma_resv_lockdep() orders the reservations inside mmap_lock. So mmap_lock is outside grbm_idx_mutex and srbm_mutex, and these two debugfs paths nest it the other way round. The copies under the locks are as old as the bank selection in both files; commit 1d0f5838b126 ("drm/amdgpu: Add lockdep annotations for lock ordering validation") only made them visible to lockdep.

Copy the user buffer in before any of the locks is taken and the result out after all of them are dropped, as amdgpu_debugfs_wave_read(), amdgpu_debugfs_gpr_read() and amdgpu_debugfs_gprwave_read() already do. The buffer is sized by the count passed to read() or write(), so an access larger than PAGE_SIZE is now rejected with -EINVAL; umr reads and writes one register at a time. A fault on the user buffer of a write now fails with -EFAULT before any register is written, instead of after a partial write.

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c |   77 ++++++++++++++++------------
 1 file changed, 46 insertions(+), 31 deletions(-)

@@ -120,6 +120,20 @@ static int  amdgpu_debugfs_process_reg_o
 
 	*pos &= (1UL << 22) - 1;
 
+	if (size > PAGE_SIZE)
+		return -EINVAL;
+
+	/*
+	 * Access the user buffer only while none of the locks below are held:
+	 * a fault takes mmap_lock, and grbm_idx_mutex and srbm_mutex nest
+	 * inside it.
+	 */
+	u32 *data __free(kfree) = kmalloc(size, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	if (!read && copy_from_user(data, buf, size))
+		return -EFAULT;
+
 	r = pm_runtime_get_sync(adev_to_drm(adev)->dev);
 	if (r < 0) {
 		pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);

@@ -151,28 +165,16 @@ static int  amdgpu_debugfs_process_reg_o
 		mutex_lock(&adev->pm.mutex);
 
 	while (size) {
-		uint32_t value;
-
-		if (read) {
-			value = RREG32(*pos >> 2);
-			r = put_user(value, (uint32_t *)buf);
-		} else {
-			r = get_user(value, (uint32_t *)buf);
-			if (!r)
-				amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, value, 0);
-		}
-		if (r) {
-			result = r;
-			goto end;
-		}
+		if (read)
+			data[result >> 2] = RREG32(*pos >> 2);
+		else
+			amdgpu_mm_wreg_mmio_rlc(adev, *pos >> 2, data[result >> 2], 0);
 
 		result += 4;
-		buf += 4;
 		*pos += 4;
 		size -= 4;
 	}
 
-end:
 	if (use_bank) {
 		amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff, 0);
 		mutex_unlock(&adev->grbm_idx_mutex);

@@ -187,6 +189,10 @@ end:
 	pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
 
 	amdgpu_virt_disable_access_debugfs(adev);
+
+	if (read && copy_to_user(buf, data, result))
+		return -EFAULT;
+
 	return result;
 }
 

@@ -237,11 +243,24 @@ static ssize_t amdgpu_debugfs_regs2_op(s
 	struct amdgpu_device *adev = rd->adev;
 	ssize_t result = 0;
 	int r;
-	uint32_t value;
 
 	if (size & 0x3 || offset & 0x3)
 		return -EINVAL;
 
+	if (size > PAGE_SIZE)
+		return -EINVAL;
+
+	/*
+	 * Access the user buffer only while none of the locks below are held:
+	 * a fault takes mmap_lock, and grbm_idx_mutex and srbm_mutex nest
+	 * inside it.
+	 */
+	u32 *data __free(kfree) = kmalloc(size, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	if (write_en && copy_from_user(data, buf, size))
+		return -EFAULT;
+
 	r = pm_runtime_get_sync(adev_to_drm(adev)->dev);
 	if (r < 0) {
 		pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);

@@ -280,24 +299,16 @@ static ssize_t amdgpu_debugfs_regs2_op(s
 		mutex_lock(&adev->pm.mutex);
 
 	while (size) {
-		if (!write_en) {
-			value = RREG32(offset >> 2);
-			r = put_user(value, (uint32_t *)buf);
-		} else {
-			r = get_user(value, (uint32_t *)buf);
-			if (!r)
-				amdgpu_mm_wreg_mmio_rlc(adev, offset >> 2, value, rd->id.xcc_id);
-		}
-		if (r) {
-			result = r;
-			goto end;
-		}
+		if (!write_en)
+			data[result >> 2] = RREG32(offset >> 2);
+		else
+			amdgpu_mm_wreg_mmio_rlc(adev, offset >> 2, data[result >> 2],
+						rd->id.xcc_id);
 		offset += 4;
 		size -= 4;
 		result += 4;
-		buf += 4;
 	}
-end:
+
 	if (rd->id.use_grbm) {
 		amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff, rd->id.xcc_id);
 		mutex_unlock(&adev->grbm_idx_mutex);

@@ -316,6 +327,10 @@ end:
 	pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
 
 	amdgpu_virt_disable_access_debugfs(adev);
+
+	if (!write_en && copy_to_user(buf, data, result))
+		return -EFAULT;
+
 	return result;
 }
 

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Jesse Zhang <[email protected]>

commit 7f7c88dbf4d543f6703e53bdeed380e886733167 upstream.

amdgpu_userq_get_doorbell_index() pins any BO passed as doorbell_handle into the doorbell domain. For a regular BO, TTM moves it with ttm_bo_move_null() and its contents are lost. Since an importer on the same device gets the exporter's GEM object, a client can do this to a buffer another client shared with it.

Reject BOs that were not created in the doorbell domain.

v2: drop the error message, use a local abo variable (Christian)

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

@@ -439,6 +439,7 @@ amdgpu_userq_get_doorbell_index(struct a
 	u64 doorbell_index;
 	struct drm_gem_object *gobj;
 	struct amdgpu_userq_obj *db_obj = db_info->db_obj;
+	struct amdgpu_bo *abo;
 	int r, db_size;
 
 	gobj = drm_gem_object_lookup(filp, db_info->doorbell_handle);

@@ -447,7 +448,17 @@ amdgpu_userq_get_doorbell_index(struct a
 		return -EINVAL;
 	}
 
-	db_obj->obj = amdgpu_bo_ref(gem_to_amdgpu_bo(gobj));
+	/*
+	 * Pinning a regular BO into the doorbell domain would discard its
+	 * contents, possibly those of a buffer shared by another client.
+	 */
+	abo = gem_to_amdgpu_bo(gobj);
+	if (!(abo->preferred_domains & AMDGPU_GEM_DOMAIN_DOORBELL)) {
+		drm_gem_object_put(gobj);
+		return -EINVAL;
+	}
+
+	db_obj->obj = amdgpu_bo_ref(abo);
 	drm_gem_object_put(gobj);
 
 	r = amdgpu_bo_reserve(db_obj->obj, true);

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Jesse Zhang <[email protected]>

commit 76ceb6ed610cbf14341a9ba78c5c0b2199b9030a upstream.

mes_userq_mqd_create() returns -ENOMEM when copying the user MQD fails, even though memdup_user() returns -EFAULT for a bad pointer or -EINTR when interrupted by a signal. Return PTR_ERR() instead.

While at it drop the error messages. Neither case is a driver error and userspace shouldn't be able to spam the log with invalid IOCTL parameters.

v2: drop the error messages (Christian)

---
 drivers/gpu/drm/amd/amdgpu/mes_userqueue.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

@@ -304,8 +304,7 @@ static int mes_userq_mqd_create(struct a
 
 		compute_mqd = memdup_user(u64_to_user_ptr(mqd_user->mqd), mqd_user->mqd_size);
 		if (IS_ERR(compute_mqd)) {
-			DRM_ERROR("Failed to read user MQD\n");
-			r = -ENOMEM;
+			r = PTR_ERR(compute_mqd);
 			goto free_mqd;
 		}
 

@@ -349,8 +348,7 @@ static int mes_userq_mqd_create(struct a
 
 		mqd_gfx_v11 = memdup_user(u64_to_user_ptr(mqd_user->mqd), mqd_user->mqd_size);
 		if (IS_ERR(mqd_gfx_v11)) {
-			DRM_ERROR("Failed to read user MQD\n");
-			r = -ENOMEM;
+			r = PTR_ERR(mqd_gfx_v11);
 			goto free_mqd;
 		}
 

@@ -394,8 +392,7 @@ static int mes_userq_mqd_create(struct a
 
 		mqd_sdma_v11 = memdup_user(u64_to_user_ptr(mqd_user->mqd), mqd_user->mqd_size);
 		if (IS_ERR(mqd_sdma_v11)) {
-			DRM_ERROR("Failed to read sdma user MQD\n");
-			r = -ENOMEM;
+			r = PTR_ERR(mqd_sdma_v11);
 			goto free_mqd;
 		}
 

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Leo Li <[email protected]>

commit 7744262f5d63f9faa4d59dec127ae1c8b2d56495 upstream.

[Why]

DM was declaring it's own list of has_alpha DRM formats to check against for enabling alpha blending, when the format->has_alpha already does that for us.

The self-declared list missed some has_alpha formats, causing direct-scanout planes with such formats to appear opaque.

[How]

Drop the self-declared list, instead use the format->has_alpha field directly.

---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c |   20 ----------------
 1 file changed, 1 insertion(+), 19 deletions(-)

@@ -110,25 +110,7 @@ void amdgpu_dm_plane_fill_blending_from_
 
 	if (plane_state->pixel_blend_mode == DRM_MODE_BLEND_PREMULTI ||
 		plane_state->pixel_blend_mode == DRM_MODE_BLEND_COVERAGE) {
-		static const uint32_t alpha_formats[] = {
-			DRM_FORMAT_ARGB8888,
-			DRM_FORMAT_RGBA8888,
-			DRM_FORMAT_ABGR8888,
-			DRM_FORMAT_ARGB2101010,
-			DRM_FORMAT_ABGR2101010,
-			DRM_FORMAT_ARGB16161616,
-			DRM_FORMAT_ABGR16161616,
-			DRM_FORMAT_ARGB16161616F,
-		};
-		uint32_t format = plane_state->fb->format->format;
-		unsigned int i;
-
-		for (i = 0; i < ARRAY_SIZE(alpha_formats); ++i) {
-			if (format == alpha_formats[i]) {
-				*per_pixel_alpha = true;
-				break;
-			}
-		}
+		*per_pixel_alpha = plane_state->fb->format->has_alpha;
 
 		if (*per_pixel_alpha && plane_state->pixel_blend_mode == DRM_MODE_BLEND_COVERAGE)
 			*pre_multiplied_alpha = false;

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Leo Li <[email protected]>

commit 2e54a92e0136c3ff5c7b542b95bf0940ad6687bf upstream.

[Why & How]

Immediate (tearing) flips breaks self refresh state machine assumptions, so disable PSR/Replay for them.

---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

@@ -10397,6 +10397,13 @@ static void amdgpu_dm_enable_self_refres
 	struct amdgpu_dm_connector *aconn =
 		(struct amdgpu_dm_connector *)acrtc_state->stream->dm_stream_context;
 
+	if (!acrtc_state->base.async_flip) {
+		amdgpu_dm_psr_set_event(dm, acrtc_state->stream, false,
+			psr_event_immediate_flip, false);
+		amdgpu_dm_replay_set_event(dm, acrtc_state->stream, false,
+			replay_event_immediate_flip, false);
+	}
+
 	/* Decrement skip count when SR is enabled and we're doing fast updates. */
 	if (acrtc_state->update_type == UPDATE_TYPE_FAST &&
 	    (psr->psr_feature_enabled || pr->replay_feature_enabled)) {

@@ -10810,6 +10817,18 @@ static void amdgpu_dm_commit_planes(stru
 			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
 		}
 		mutex_lock(&dm->dc_lock);
+		/*
+		 * Tearing (immediate) flips cannot work with panel self-refresh
+		 * features. The event is cleared by
+		 * amdgpu_dm_enable_self_refresh() once async flips stop.
+		 */
+		if (immediate_flip) {
+			dc_exit_ips_for_hw_access(dm->dc);
+			amdgpu_dm_psr_set_event(dm, acrtc_state->stream, true,
+				psr_event_immediate_flip, true);
+			amdgpu_dm_replay_set_event(dm, acrtc_state->stream, true,
+				replay_event_immediate_flip, true);
+		}
 		update_planes_and_stream_adapter(dm->dc,
 					 acrtc_state->update_type,
 					 planes_count,

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 1ab8904ae61b076d1242e1f48537698c77074b5c upstream.

It's groundhog day.

MNT_UNBINDABLE used to be set in in mnt->mnt.mnt_flags and was not part of MNT_INTERNAL_FLAGS. clone_mnt() copied it with all the other flags to the new mount. This guaranteed that the copy of an unbindable mount became unbindable as well. Unbindable copies that ended up as shared lost the unbindable property.

But then commit 406fea799925 ("mount: separate the flags accessed only under namespace_sem") moved MNT_UNBINDABLE from mnt->mnt.mnt_flags into mnt->mnt_t_flags as T_UNBINDABLE.

clone_mnt() doesn't copy from mnt_t_flags and specifically doesn't copy T_UNBINDABLE. The only place a mount gets marked unbindable is in change_mnt_propagation() for MS_UNBINDABLE.

This reintroduced an earlier bug we had already fixed. It resurfaces in copy_mnt_ns() which clones unbindable mounts via CL_COPY_UNBINDABLE. So since v6.17 a mount namespace created via clone(CLONE_NEWNS) or unshare(CLONE_NEWNS) contain private and bindable copies of every unbindable mount. The following snippet:

  mount --make-unbindable /mnt
  unshare -m --propagation unchanged
  mount --bind /mnt /tmp/x

succeeds. This used to fail with EINVAL. This also applies to recursively binding a tree that contains an unbindable the mount. Such subtrees used to be pruned when they were moved onto a shared mount.

And Documentation/filesystems/sharedsubtree.rst still says that the copy of an unbindable mount is unbindable.

So once again let's fix this. Copy T_UNBINDABLE in clone_mnt(). Let set_mnt_shared() mask it off whenever a mount is made shared. This restores what the old MNT_INTERNAL_FLAGS mask did.

---
 fs/namespace.c |    1 +
 1 file changed, 1 insertion(+)

@@ -1254,6 +1254,7 @@ static struct mount *clone_mnt(struct mo
 
 	mnt->mnt.mnt_flags = READ_ONCE(old->mnt.mnt_flags) &
 			     ~MNT_INTERNAL_FLAGS;
+	mnt->mnt_t_flags = old->mnt_t_flags & T_UNBINDABLE;
 
 	if (flag & (CL_SLAVE | CL_PRIVATE))
 		mnt->mnt_group_id = 0; /* not a peer of original */

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 6857e209015f95be7a8f19a9cd5dac345b9b9859 upstream.

The propagate_mount_busy() helper exists to decide whether a synchronous can suceed. It does that by looking at the copies of the victims in the mounts its parent propagates to. One of those functions from propagation hell.

It skips a copy that has child mounts except for the case of a single child mount covering the copy's root. That algorithm used to be exact. It isn't anymore.

Originally only childless copies were unmounted during propagation. And 1064f874abc0 ("mnt: Tuck mounts under others instead of creating shadow/side mounts.") ended up adding covered mounts to both sides in one go.

Along came 99b19d16471e ("mnt: In propgate_umount handle visiting mounts in any order") and that started to unmount more than before:

[1]: A copy of a mount is unmounted when each of its children is either
its overmount or a copy of the victim itself. In the former case the overmount gets reparented. In the latter case the copy of the victim will end up being unmounted.

So propagate_umount() ended up being rewritten without adjusting propagate_mount_busy(). And currently propagate_mount_busy() doesn't look at copies of the victim that have two or more children nor a single child that is not the overmount.

So any such copy in [1] is not checked for
references. The result is that a synchronous unmount succeeds, pulling out a mount that is still in use.

A container can run into this with its own mounts. The host shares a tree with the container and the host has a mount in that tree:

  mount -t tmpfs none /mnt
  mount --make-shared /mnt
  mount -t tmpfs none /mnt/a

The container's /mnt is a slave copy of the host mount. The container now takes a detached copy of the tree and mounts that copy beneath its propagated copy of /mnt/a and keeps the file descriptor to the tree open:

  unshare -m --propagation unchanged
  mount --make-rslave /
  fd = open_tree(AT_FDCWD, "/mnt", OPEN_TREE_CLONE | AT_RECURSIVE)
  move_mount(fd, "", AT_FDCWD, "/mnt/a", MOVE_MOUNT_BENEATH)

grep /mnt /proc/self/mountinfo

  775 355 ... /mnt ...     master:646
  777 775 ... /mnt/a ...   master:646
  776 777 ... /mnt/a ...   master:647

778 777 ... /mnt/a/a ... master:647

The now attached tree (777) is now mounted beneath the container's copy of /mnt/a. That's also where the host mount sits (as seen from the host mount namespace). So the propagated copy (776) of the host's mount is on top of its root and the copy of that (778) is inside it. Now the host runs:

umount /mnt/a

and it succeeds. The moved tree tree and the copy inside of it are gone from the container. Now only the propagated copy is left and it got reparented to where it was before:

grep /mnt /proc/self/mountinfo
775 355 ... /mnt ... master:646
776 775 ... /mnt/a ...

But the file descriptor still refers to the moved tree. So a synchronous umount detached a mount that is still in use.

Teach propagate_mount_busy() the same checks that propagate_umount() does. It now checks for a single victim mount without children for which it can assume that MNT_LOCKED is cleared on every copy like propagate_umount() does. The copies form chains. IOW, when a copy itself receives propagation from the victim's parent then the mount at the same mountpoint inside that copy is a copy as well. Whether a mount receives propagation from the victim's can simply be checked by walking its masters. Walk each chain once from its top so we are sure that nested copies stay linear. Check every reference once of every copy that gets unmounted.

---
 fs/pnode.c |  110 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 91 insertions(+), 19 deletions(-)

@@ -410,19 +410,99 @@ bool propagation_would_overmount(const s
 	return false;
 }
 
+/* Does @m receive propagation from @parent? */
+static bool receives_from(struct mount *m, struct mount *parent)
+{
+	if (m == parent)
+		return false;
+	for (; m; m = m->mnt_master)
+		if (m == parent || peers(m, parent))
+			return true;
+	return false;
+}
+
+/*
+ * Does @m receive propagation from the victim's parent as well? If so, then
+ * the mount at the victim's mountpoint inside of @m is a umount candidate as
+ * well. So it's the next candidate in the chain. Otherwise the chain ends at
+ * @m.
+ */
+static struct mount *next_candidate(struct mount *m, struct mount *victim)
+{
+	if (!receives_from(m, victim->mnt_parent))
+		return NULL;
+	return __lookup_mnt(&m->mnt, victim->mnt_mountpoint);
+}
+
+/*
+ * Would propagate_umount() pull out a mount of the chain of candidates that
+ * starts at @c, and does that mount have references beyond its own?
+ *
+ * This mirrors how trim_one(), trim_ancestors() and handle_locked() handle a
+ * synchronous umount:
+ *
+ * - single victim
+ * - without children
+ * - with MNT_LOCKED already cleared on every candidate by propagate_mount_unlock()
+ *
+ * A copy of the victim gets unmounted when each of its children is
+ * the next candidate in the chain or its overmount, unless the next
+ * unmount candidate is not its overmount and some unmount candidate further
+ * down has a child outside the chain.  Keep this in sync with
+ * Documentation/filesystems/propagate_umount.txt.
+ */
+static bool chain_busy(struct mount *c, struct mount *victim)
+{
+	struct mount *m, *n, *next, *deepest = NULL;
+	bool above;
+
+	/* the deepest candidate with a child outside the chain */
+	for (m = c; m; m = next) {
+		next = next_candidate(m, victim);
+		list_for_each_entry(n, &m->mnt_mounts, mnt_child) {
+			if (n != next && n != victim) {
+				deepest = m;
+				break;
+			}
+		}
+	}
+
+	above = deepest != NULL;	/* @deepest is at or below @m */
+	for (m = c; m; m = next) {
+		bool goes = true;
+
+		next = next_candidate(m, victim);
+		list_for_each_entry(n, &m->mnt_mounts, mnt_child) {
+			if (n != next && n != m->overmount && n != victim) {
+				goes = false;
+				break;
+			}
+		}
+		if (goes && next && next != m->overmount && above && m != deepest)
+			goes = false;
+		if (m == deepest)
+			above = false;
+		if (goes && do_refcount_check(m, 1))
+			return true;
+	}
+	return false;
+}
+
 /*
  * check if the mount 'mnt' can be unmounted successfully.
  * @mnt: the mount to be checked for unmount
  * NOTE: unmounting 'mnt' would naturally propagate to all
  * other mounts its parent propagates to.
- * Check if any of these mounts that **do not have submounts**
- * have more references than 'refcnt'. If so return busy.
+ * Check if any of the mounts that propagate_umount() would pull out
+ * along with it have more references than their own. If so return busy.
  *
  * vfsmount lock must be held for write
  */
 int propagate_mount_busy(struct mount *mnt, int refcnt)
 {
 	struct mount *parent = mnt->mnt_parent;
+	struct dentry *mp = mnt->mnt_mountpoint;
+	struct mount *m;
 
 	/*
 	 * quickly check if the current mount can be unmounted.

@@ -435,24 +515,16 @@ int propagate_mount_busy(struct mount *m
 	if (mnt == parent)
 		return 0;
 
-	for (struct mount *m = propagation_next(parent, parent); m;
-	     		m = propagation_next(m, parent)) {
-		struct list_head *head;
-		struct mount *child = __lookup_mnt(&m->mnt, mnt->mnt_mountpoint);
-
-		if (!child)
+	/* the candidates are the mounts at @mp below the receivers */
+	for (m = propagation_next(parent, parent); m;
+	     m = propagation_next(m, parent)) {
+		struct mount *c = __lookup_mnt(&m->mnt, mp);
+
+		/* each chain once, from its top: skip receivers that are candidates */
+		if (!c || (mnt_has_parent(m) && m->mnt_mountpoint == mp &&
+			   receives_from(m->mnt_parent, parent)))
 			continue;
-
-		head = &child->mnt_mounts;
-		if (!list_empty(head)) {
-			/*
-			 * a mount that covers child completely wouldn't prevent
-			 * it being pulled out; any other would.
-			 */
-			if (!list_is_singular(head) || !child->overmount)
-				continue;
-		}
-		if (do_refcount_check(child, 1))
+		if (chain_busy(c, mnt))
 			return 1;
 	}
 	return 0;

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 476ef286a7a5db8bda51325dc0c45887b4d99d9a upstream.

__umount_mnt() resets ->overmount of the parent when it unmounts the mount that overmounts it. mnt_change_mountpoint() doesn't. It moves the mount to its new parent and leaves the old one pointing at it.

For a mount that is moved beneath in attach_recursive_mnt() that doesn't matter. The new mount has already reset ->overmount of the old parent by the time the old top mount is moved onto it.

It matters for reparent() though. propagate_umount() moves a surviving overmount onto the first ancestor that isn't going to be unmounted. The mount it was taken from is unmounted immediately afterwards. If a file descriptor keeps that unmounted mount alive ->overmount points at the moved mount. That's a dangling pointer and UAF as the moved mount is unmounted and freed.

do_lock_mount() follows ->overmount from the target path under mount_lock. With a file descriptor on such a mount as the target of a mount beneath topmost_overmount() walks into the freed mount and do_lock_mount() takes references on whatever it finds there.

So reset ->overmount of the old parent in mnt_change_mountpoint() the same way __umount_mnt() does. A mount's ->overmount is then always a mount that is attached to it.

---
 fs/namespace.c |    3 +++
 1 file changed, 3 insertions(+)

@@ -1058,7 +1058,10 @@ static void attach_mnt(struct mount *mnt
 void mnt_change_mountpoint(struct mount *parent, struct mountpoint *mp, struct mount *mnt)
 {
 	struct mountpoint *old_mp = mnt->mnt_mp;
+	struct mount *old_parent = mnt->mnt_parent;
 
+	if (old_parent->overmount == mnt)
+		old_parent->overmount = NULL;
 	list_del_init(&mnt->mnt_child);
 	hlist_del_init(&mnt->mnt_mp_list);
 	hlist_del_init_rcu(&mnt->mnt_hash);

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit bb46094057520304073130920c223451c2a694a7 upstream.

statmount_mnt_basic() reads the ids of the parent through m->mnt_parent with namespace_sem held for reading. That's fine for a mount that is in a namespace. Everything above it is pinned by that namespace.

STATMOUNT_BY_FD also accepts a file descriptor on a mount that has been unmounted on purpose and that only the file descriptor keeps alive. If that mount stayed attached to its parent when it was unmounted the parent is unmounted as well and nothing pins it for the child.

The final mntput() unmounts the children under mount_lock alone and frees the parent afterwards. Concurrently, statmount_mnt_basic() reads m->mnt_parent twice while that can happen and can easily dereference a mount that's already gone.

Read the parent of an unmounted mount under mount_lock. The parent can't be put while it is held and both ids come from the same mount. A mounted mount's parent is pinned by the namespace and a mount can't leave its namespace while namespace_sem is held, so the common case takes no global lock.

---
 fs/namespace.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

@@ -5316,15 +5316,25 @@ static void statmount_sb_basic(struct ks
 	s->sm.sb_flags = sb->s_flags & (SB_RDONLY|SB_SYNCHRONOUS|SB_DIRSYNC|SB_LAZYTIME);
 }
 
+static void statmount_mnt_parent(struct kstatmount *s, const struct mount *m)
+{
+	s->sm.mnt_parent_id = m->mnt_parent->mnt_id_unique;
+	s->sm.mnt_parent_id_old = m->mnt_parent->mnt_id;
+}
+
 static void statmount_mnt_basic(struct kstatmount *s)
 {
 	struct mount *m = real_mount(s->mnt);
 
 	s->sm.mask |= STATMOUNT_MNT_BASIC;
 	s->sm.mnt_id = m->mnt_id_unique;
-	s->sm.mnt_parent_id = m->mnt_parent->mnt_id_unique;
 	s->sm.mnt_id_old = m->mnt_id;
-	s->sm.mnt_parent_id_old = m->mnt_parent->mnt_id;
+	/* An unmounted mount is cut loose from its parent under mount_lock alone. */
+	if (likely(is_mounted(s->mnt)))
+		statmount_mnt_parent(s, m);
+	else
+		scoped_guard(mount_locked_reader)
+			statmount_mnt_parent(s, m);
 	s->sm.mnt_attr = mnt_to_attr_flags(&m->mnt);
 	s->sm.mnt_propagation = mnt_to_propagation_flags(m);
 	s->sm.mnt_peer_group = m->mnt_group_id;

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit e2c6d326216642011da318b9edfd5d57ebd5a56f upstream.

MNT_UMOUNT gets raised on a mount that umount_tree() or umount_one() has taken out of its namespace. It's never cleared. __detach_mounts() unhooks a mount that carries it instead of unmounting it.

disconnect_mount() lets an unmounted child stay attached only if its parent carries it. propagate_umount() takes it as the sign that a mount is going to be unmounted when it decides what else can be unmounted, where a chain of locked mounts ends and where a for reparenting an overmount.

clone_mnt() copies the flags of the source and only masks MNT_INTERNAL_FLAGS which doesn't include MNT_UMOUNT. A mount that was lazily unmounted but is kept alive by a file descriptor can still be copied when its root is an nsfs or pidfs file because may_copy_tree() accepts those before checking whether the source is mounted at all:

  mount --bind /proc/self/ns/net /tmp/x
  exec 3< /tmp/x
  umount -l /tmp/x
  mount --bind /proc/self/fd/3 /tmp/y

The mount on /tmp/y is attached to the caller's namespace with MNT_UMOUNT already set and every copy of it inherits the flag again.

That causes a number of bugs:

- Unlink its mountpoint from a mount namespace where it isn't one. __detach_mounts() takes it for a mount that was unmounted earlier, unhooks it from its parent and drops the reference the namespace holds on it. It stays in the namespace with one reference too few and as its own parent, so it can't be unmounted through the file descriptor anymore and the namespace never finds it again. When the namespace goes away it's leaked and points to a freed struct mnt_namespace that may_change_propagation() and anon_ns_root() read on the next propagation change or mount_setattr() through the file descriptor.

- Make it shared and unmount a peer. trace_transfers() follows the flag from the peer into it and strips it from its peer group.

- Let a propagated umount reach it. gather_candidates() marks it with T_UMOUNT_CANDIDATE but never puts it on the candidate list, so the mark is never cleared and every later propagated umount skips it. If it's the parent of a locked mount handle_locked() takes it as the cutoff and commits the locked child that should have stayed and disconnect_mount() leaves that child attached to it, unmounted under a mounted parent, which is what disconnect_mount() exists to prevent. When the namespace goes away umount_tree() visits the child a second time. And once __detach_mounts() has made it its own parent reparent() never finds the end of the chain of unmounted parents.

A clone is a new mount that has never been unmounted. Add MNT_UMOUNT to MNT_INTERNAL_FLAGS so clone_mnt() never copies it. And warn when umount_tree() reaches a mount that already has it set.

---
 fs/namespace.c        |    2 ++
 include/linux/mount.h |    2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

@@ -1787,6 +1787,8 @@ static void umount_tree(struct mount *mn
 
 	/* Gather the mounts to umount */
 	for (p = mnt; p; p = next_mnt(p, mnt)) {
+		/* A mount is unmounted once. */
+		VFS_WARN_ON_ONCE(p->mnt.mnt_flags & MNT_UMOUNT);
 		p->mnt.mnt_flags |= MNT_UMOUNT;
 		if (mnt_ns_attached(p))
 			move_from_ns(p);

@@ -52,7 +52,7 @@ enum mount_flags {
 	MNT_ATIME_MASK = MNT_NOATIME | MNT_NODIRATIME | MNT_RELATIME,
 
 	MNT_INTERNAL_FLAGS = MNT_INTERNAL | MNT_DOOMED |
-			     MNT_SYNC_UMOUNT | MNT_LOCKED
+			     MNT_SYNC_UMOUNT | MNT_LOCKED | MNT_UMOUNT
 };
 
 struct vfsmount {

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 8bacbf6be959973b28b691713a872c12eae69eec upstream.

may_copy_tree() accepts a mount whose root is an nsfs or pidfs file before it checks whether that mount is mounted. This exception exists for nsfs and pidfs mounts. They are part of no mount namespace so neither check_mnt() nor is_mounted() would let them through and bind-mounting a nsfsfd or a pidfd wouldn't work.

Problem is that exception also admits a bind mount of such a file when it was lazily unmounted and is only kept alive by a file descriptor:

  mount --bind /proc/self/ns/net /tmp/x
  exec 3< /tmp/x
  umount -l /tmp/x
  mount --rbind /proc/self/fd/3 /tmp/y

With MS_REC or AT_RECURSIVE copy_tree() doesn't stop at the unmounted mount. It copies every mount that stayed attached to it as well. So if you have a bunch of overmounts over the nsfsfd or the pidfd the whole unmounted stack is copied. That's a bug.

So refuse to walk an unmounted tree. Copying its root alone stays possible. So a bind mount of a namespace that is gone can still be recreated from a file descriptor on the old one. The file descriptor pins that mount and its root and __has_locked_children() only reads the direct children which don't leave the parent's list without namespace_sem. The internal mounts carry MNT_NS_INTERNAL instead of NULL and are still walked. Copying an unmounted tree recursively fails with EINVAL.

---
 fs/namespace.c |    3 +++
 1 file changed, 3 insertions(+)

@@ -2992,6 +2992,9 @@ static struct mount *__do_loopback(const
 	if (!may_copy_tree(old_path))
 		return ERR_PTR(-EINVAL);
 
+	if (recurse && !old->mnt_ns)
+		return ERR_PTR(-EINVAL);
+
 	if (!recurse && __has_locked_children(old, old_path->dentry))
 		return ERR_PTR(-EINVAL);
 

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 02587a4af82adccaef8e3b3624b4e7f1c401632d upstream.

There's a few instances where a internal superblock is reachable by fspick(). Block that. graft_tree() and fsmount() already refuse them. fanotify refuses mount and superblock marks on them for the same reason.

---
 fs/fsopen.c |    3 +++
 1 file changed, 3 insertions(+)

@@ -190,6 +190,9 @@ SYSCALL_DEFINE3(fspick, int, dfd, const
 	ret = -EINVAL;
 	if (target.mnt->mnt_root != target.dentry)
 		goto err_path;
+	/* kernel-internal superblocks are nobody's to reconfigure */
+	if (target.dentry->d_sb->s_flags & SB_NOUSER)
+		goto err_path;
 
 	fc = fs_context_for_reconfigure(target.dentry, 0, 0);
 	if (IS_ERR(fc)) {

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 08f4288a37ef4b3d19b4a9599ec3048219cac828 upstream.

When mount_setattr() changes the propagation type to MS_SHARED do_mount_setattr() takes namespace_sem and calls invent_group_ids() which walks the whole target tree with next_mnt() and allocates a peer group id for every mount that doesn't have one.

Whether the caller may touch the target at all is only checked afterwards under mount_lock. If it may not, cleanup_group_ids() walks the tree a second time on the way out to free the ids again.

A detached tree that a file descriptor keeps alive, a mount in another mount namespace or a mount below the root of an anonymous mount namespace does a bunch of allocation work that is completely useless because it will fail and revert right after.

Do the right thing and check the target right after taking namespace_sem instead. A mount leaves its namespace only under namespace_sem so the mount stays stable. The check under mount_lock stays for the case where no propagation change is requested and namespace_sem isn't taken. Callers get the same EINVAL as before.

---
 fs/namespace.c |    6 ++++++
 1 file changed, 6 insertions(+)

@@ -4957,6 +4957,11 @@ static int do_mount_setattr(const struct
 		 * propagation.
 		 */
 		namespace_lock();
+		/* invent_group_ids() walks the tree, only walk a mounted one */
+		if (!anon_ns_root(mnt) && !check_mnt(mnt)) {
+			namespace_unlock();
+			return -EINVAL;
+		}
 		if (kattr->propagation == MS_SHARED) {
 			err = invent_group_ids(mnt, kattr->kflags & MOUNT_KATTR_RECURSE);
 			if (err) {

@@ -4969,6 +4974,7 @@ static int do_mount_setattr(const struct
 	err = -EINVAL;
 	lock_mount_hash();
 
+	/* Checked under namespace_sem already if the propagation changes. */
 	if (!anon_ns_root(mnt) && !check_mnt(mnt))
 		goto out;
 

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit bd2feb7796b6585938a4241e7b7c4148a6f7b762 upstream.

mnt_notify_add() puts a mount on notify_list. It doesn't check whether the mount is on the list already. That's fine as long as every mount is queued at most once per namespace_sem hold. It isn't.

propagate_umount() moves a surviving overmount off a stack of mounts that are going away and queues it as moved. shrink_submounts() and mark_mounts_for_expiry() call umount_tree() for several mounts under a single namespace_sem hold. So the second umount_tree() can take down exactly the mount the first one reparented, reparent it once more and so end up queueing it a second time.

The second list_add_tail() cuts the mounts queued in between out of the list while the head still points at the last of them. notify_mnt_list() then only visits that one mount and the others are freed after the grace period with notify_list still pointing at them. Every later mount operation on the host walks freed memory:

  BUG: KASAN: slab-use-after-free in __list_add_valid_or_report
  Read of size 8 at addr ffff8881003d57e0 by task notify_dq/162
   __list_add_valid_or_report+0x15c/0x1a0
   mnt_add_to_ns+0x322/0x890
   attach_recursive_mnt.isra.0+0xf2a/0x1a00
   path_mount+0x139c/0x1d40

This needs a mount with MNT_SHRINKABLE to bind from. That's what finish_automount() creates (submounts of NFS, AFS and CIFS and the tracefs mount below debugfs). Bind mounts inherit the flag. A mount namespace with a FAN_MARK_MNTNS mark does the rest and root in a user namespace can have both.

Initialize to_notify in alloc_vfsmnt() and leave a mount alone that is queued already. mnt_notify() looks at the state the mount has when the list is drained so a single entry per mount is enough. The move event for a mount that is unmounted under the same hold is lost. That's fine. The detach is what matters.

---
 fs/mount.h     |    3 +++
 fs/namespace.c |    3 +++
 2 files changed, 6 insertions(+)

@@ -233,6 +233,9 @@ static inline struct mnt_namespace *to_m
 #ifdef CONFIG_FSNOTIFY
 static inline void mnt_notify_add(struct mount *m)
 {
+	/* queued already under this namespace_sem hold */
+	if (!list_empty(&m->to_notify))
+		return;
 	/* Optimize the case where there are no watches */
 	if ((m->mnt_ns && m->mnt_ns->n_fsnotify_marks) ||
 	    (m->prev_ns && m->prev_ns->n_fsnotify_marks))

@@ -322,6 +322,9 @@ static struct mount *alloc_vfsmnt(const
 		INIT_HLIST_NODE(&mnt->mnt_mp_list);
 		INIT_HLIST_HEAD(&mnt->mnt_stuck_children);
 		INIT_HLIST_NODE(&mnt->mnt_ns_visible);
+#ifdef CONFIG_FSNOTIFY
+		INIT_LIST_HEAD(&mnt->to_notify);
+#endif
 		RB_CLEAR_NODE(&mnt->mnt_node);
 		mnt->mnt.mnt_idmap = &nop_mnt_idmap;
 	}

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 32bf92416614a1e3ff3524fc6b780259c7aad3ef upstream.

shrink_submounts() and mark_mounts_for_expiry() first collect all the mounts they are allowed to unmount and then unmount them.

Whether a mount is busy is decided by propagate_mount_busy() on the tree. But unmounting the first mount changes the tree that the second one propagates into. propagate_umount()
moves a surviving overmount off a copy it unmounts and mounts it where the copy was mounted. If that's where the propagated copy of the second victim is looked up the overmount becomes a candidate of the second umount_tree(). It's childless and so trim_one() commits it without looking at its reference count.

So a synchronous umount pulls out a busy mount that is in use somewhere else and marks it MNT_SYNC_UMOUNT while it has users:

umount2(/tmp/plshrink/p1, 0) = 0 errno 0
cwd is (unreachable)

The same two umounts requested one after the other fail with EBUSY. This needs a mount with MNT_SHRINKABLE, i.e., automounted submounts of NFS, AFS and CIFS or the tracefs mount below debugfs. Bind mounts inherit the flag.

Check each mount right before it is unmounted under the same hold of namespace_sem and mount_lock as the umount itself. Make sure that the algorithm stays linear.

---
 fs/namespace.c |   78 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 25 deletions(-)

@@ -3965,6 +3965,11 @@ void mark_mounts_for_expiry(struct list_
 	}
 	while (!list_empty(&graveyard)) {
 		mnt = list_first_entry(&graveyard, struct mount, mnt_expire);
+		/* an earlier umount_tree() may have moved a busy mount here */
+		if (propagate_mount_busy(mnt, 1)) {
+			list_move(&mnt->mnt_expire, mounts);
+			continue;
+		}
 		touch_mnt_namespace(mnt->mnt_ns);
 		umount_tree(mnt, UMOUNT_PROPAGATE|UMOUNT_SYNC);
 	}

@@ -3973,16 +3978,36 @@ void mark_mounts_for_expiry(struct list_
 EXPORT_SYMBOL_GPL(mark_mounts_for_expiry);
 
 /*
+ * Unmount @mnt if it's a shrinkable mount without children that nobody uses.
+ *
+ * mount_lock must be held for write
+ */
+static bool shrink_submount(struct mount *mnt)
+{
+	if (propagate_mount_busy(mnt, 1))
+		return false;
+	touch_mnt_namespace(mnt->mnt_ns);
+	umount_tree(mnt, UMOUNT_PROPAGATE|UMOUNT_SYNC);
+	return true;
+}
+
+/*
  * Ripoff of 'select_parent()'
  *
- * search the list of submounts for a given mountpoint, and move any
- * shrinkable submounts to the 'graveyard' list.
+ * unmount the shrinkable submounts of @parent that aren't busy, children
+ * before their parent, and say whether anything went
+ *
+ * The cursor into the children of @this_parent survives the umount of a
+ * child mount without child mounts. The mounts that get umounted together with
+ * it are located under receiving mounts of @this_parent and never under
+ * @this_parent itself. The one exception is @this_parent getting unmounted
+ * then the walk starts over.
  */
-static int select_submounts(struct mount *parent, struct list_head *graveyard)
+static bool __shrink_submounts(struct mount *parent)
 {
 	struct mount *this_parent = parent;
 	struct list_head *next;
-	int found = 0;
+	bool shrunk = false;
 
 repeat:
 	next = this_parent->mnt_mounts.next;

@@ -4001,42 +4026,45 @@ resume:
 			this_parent = mnt;
 			goto repeat;
 		}
-
-		if (!propagate_mount_busy(mnt, 1)) {
-			list_move_tail(&mnt->mnt_expire, graveyard);
-			found++;
-		}
+		if (!shrink_submount(mnt))
+			continue;
+		shrunk = true;
+		if (unlikely(this_parent->mnt.mnt_flags & MNT_UMOUNT))
+			return true;
 	}
 	/*
 	 * All done at this level ... ascend and resume the search
 	 */
 	if (this_parent != parent) {
-		next = this_parent->mnt_child.next;
-		this_parent = this_parent->mnt_parent;
+		struct mount *mnt = this_parent;
+
+		next = mnt->mnt_child.next;
+		this_parent = mnt->mnt_parent;
+		/* its children are gone, maybe it can go as well */
+		if (shrink_submount(mnt)) {
+			shrunk = true;
+			if (unlikely(this_parent->mnt.mnt_flags & MNT_UMOUNT))
+				return true;
+		}
 		goto resume;
 	}
-	return found;
+	return shrunk;
 }
 
 /*
- * process a list of expirable mountpoints with the intent of discarding any
- * submounts of a specific parent mountpoint
+ * unmount the shrinkable submounts of @mnt that aren't busy
+ *
+ * The busy check and the umount of a mount are adjacent. An umount can
+ * still empty or move a mount in a part of the tree that was walked
+ * already, so walk again until nothing goes.
  *
  * mount_lock must be held for write
  */
 static void shrink_submounts(struct mount *mnt)
 {
-	LIST_HEAD(graveyard);
-	struct mount *m;
-
-	/* extract submounts of 'mountpoint' from the expiration list */
-	while (select_submounts(mnt, &graveyard)) {
-		while (!list_empty(&graveyard)) {
-			m = list_first_entry(&graveyard, struct mount,
-						mnt_expire);
-			touch_mnt_namespace(m->mnt_ns);
-			umount_tree(m, UMOUNT_PROPAGATE|UMOUNT_SYNC);
-		}
+	for (;;) {
+		if (!__shrink_submounts(mnt))
+			break;
 	}
 }
 

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit c640a01a78e42302879006ed7b7d5a9a943ba815 upstream.

do_loopback() refuses to bind mount the file of a mount namespace that is as old as the caller's or older because a mount namespace that holds a mount of its own file, or of an ancestor's, would cause a cycle. But it only checks the dentry the bind mount starts from. With MS_REC everything below it is copied and do_loopback() passes CL_COPY_MNT_NS_FILE so mount namespace files below the source are copied.

That's fine for a source in the caller's own mount namespace. Every mount namespace file in there passed the same check when it was bind-mounted. It isn't fine for a source in another mount namespace. may_copy_tree() accepts a bind mount of any nsfs or pidfs file no matter what mount namespace it lives in so that /proc/<pid>/ns/<ns> and pidfds can be bind mounted. If we create a bind-mount stack of mount namespaces file descriptors the kernel will copy them irrespective of their ancestoral relationship to the mount namespace in question:

151 146 0:7 net:[4026531833] /tmp/nrc/y rw - nsfs nsfs rw 152 151 0:7 mnt:[4026532293] /tmp/nrc/y rw - nsfs nsfs rw

Mount 152 is a mount of the mount namespace file of mount namespace 4026532293 inside mount namespace 4026532293. That namespace and every mount in it are leaked... All it takes is a process in an older mount namespace that stacks the file and repeating it leaks without limit:

after control: Shmem: 380 kB
child: mount(/proc/self/fd/6, MS_BIND|MS_REC): ok
after cycle: Shmem: 65916 kB

do_move_mount() runs check_for_nsfs_mounts() over a detached tree for exactly that reason. Do the same for the copy before it is grafted.

---
 fs/namespace.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

@@ -3037,7 +3037,11 @@ static int do_loopback(const struct path
 	if (IS_ERR(mnt))
 		return PTR_ERR(mnt);
 
-	err = graft_tree(mnt, &mp);
+	/* the copy may carry mount namespace files from below the source */
+	if (recurse && !check_for_nsfs_mounts(mnt))
+		err = -EINVAL;
+	else
+		err = graft_tree(mnt, &mp);
 	if (err) {
 		lock_mount_hash();
 		umount_tree(mnt, UMOUNT_SYNC);

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 0febe278ec45b1ccdbe9bd5ff1ba365d8fd55e2a upstream.

open_tree(OPEN_TREE_NAMESPACE) creates a new mount namespace from a copy of the tree at the given path. The caller needs to be privileged over its current user namespace because the new mount namespace will be owned by it. It doesn't need to be privileged over the mount namespace the tree is copied from. An unprivileged user just needs to create a user namespace first.

The copy follows the rules of a detached bind mount. Without AT_RECURSIVE only the mount itself is cloned and its children are left out. With AT_RECURSIVE unbindable mounts are skipped. Both is fine when the caller has privileges over the source mount namespace because it could unmount those mounts anyway. Not so for an unprivileged user in that mount namespace. For them a mount namespace copy is what unshare() does. copy_mnt_ns() copies everything including unbindable mounts and lock_mnt_tree() makes sure nothing can be unmounted in the copy. Nothing that was covered gets revealed.

create_new_namespace() only does the locking and so the covered content is right there in the new mount namespace:

  # mount -t tmpfs none /tmp/otn; mkdir /tmp/otn/covered
  # echo hidden > /tmp/otn/covered/under.txt
  # mount -t tmpfs none /tmp/otn/covered

  uid 1000, after unshare(CLONE_NEWUSER):
  fd = open_tree(AT_FDCWD, "/tmp/otn", OPEN_TREE_NAMESPACE);
  setns(fd, CLONE_NEWNS);
  open("/covered/under.txt", O_RDONLY)  -> "hidden"

Covering paths with mounts is how container runtimes mask parts of /proc and /sys and how admins hide things.

When the caller's user namespace doesn't own the source mount namespace copy the way unshare() does and refuse a non-recursive copy of a mount that has anything mounted below the requested directory and copy unbindable mounts in a recursive copy. A caller that is privileged over the source mount namespace sees no change.

---
 fs/namespace.c |   30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

@@ -2354,6 +2354,18 @@ bool has_locked_children(struct mount *m
 	return __has_locked_children(mnt, dentry);
 }
 
+/* locks: namespace_shared && pinned(mnt) || mount_locked_reader */
+static bool __has_children(struct mount *mnt, struct dentry *dentry)
+{
+	struct mount *child;
+
+	list_for_each_entry(child, &mnt->mnt_mounts, mnt_child) {
+		if (is_subdir(child->mnt_mountpoint, dentry))
+			return true;
+	}
+	return false;
+}
+
 /*
  * Check that there aren't references to earlier/same mount namespaces in the
  * specified subtree.  Such references can act as pins for mount namespaces

@@ -3123,12 +3135,18 @@ static struct mnt_namespace *create_new_
 	struct mount *mnt;
 	unsigned int copy_flags = 0;
 	bool locked = false, recurse = flags & MOUNT_COPY_RECURSIVE;
+	bool foreign = user_ns != ns->user_ns;
 
 	if (unlikely(!d_can_lookup(path->dentry)))
 		return ERR_PTR(-ENOTDIR);
 
-	if (user_ns != ns->user_ns)
-		copy_flags |= CL_SLAVE;
+	/*
+	 * Without privileges over the mount namespace the copy is made from
+	 * nothing mounted below @path may be left out. It would reveal what
+	 * it covers. That's what unshare() gives such a caller as well.
+	 */
+	if (foreign)
+		copy_flags |= CL_SLAVE | CL_COPY_UNBINDABLE;
 
 	new_ns = alloc_mnt_ns(user_ns, false);
 	if (IS_ERR(new_ns))

@@ -3162,10 +3180,14 @@ static struct mnt_namespace *create_new_
 	/*
 	 * We don't emulate unshare()ing a mount namespace. We stick to
 	 * the restrictions of creating detached bind-mounts. It has a
-	 * lot saner and simpler semantics.
+	 * lot saner and simpler semantics. A caller without privileges
+	 * over the mount namespace can't leave out any child though.
 	 */
 	if (flags & MOUNT_COPY_NEW)
 		mnt = clone_mnt(real_mount(path->mnt), path->dentry, copy_flags);
+	else if (foreign && !recurse &&
+		 __has_children(real_mount(path->mnt), path->dentry))
+		mnt = ERR_PTR(-EINVAL);
 	else
 		mnt = __do_loopback(path, recurse, copy_flags);
 	scoped_guard(mount_writer) {

@@ -3182,7 +3204,7 @@ static struct mnt_namespace *create_new_
 		 * of the real rootfs we created.
 		 */
 		attach_mnt(mnt, new_ns_root, mp.mp);
-		if (user_ns != ns->user_ns)
+		if (foreign)
 			lock_mnt_tree(new_ns_root);
 	}
 

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 8714b44adac7b10506e1db3ef1710689751f0f10 upstream.

attach_recursive_mnt() preallocates a mountpoint for the root of the topmost mount of the source so that a mount that already sits at the destination can be put on top of the source or on top of one of its propagated copies.

The copies are made without CL_COPY_MNT_NS_FILE so their chain of overmounts is shorter than the source's. It ends below the first bind mount of a mount namespace file. So while the loop walks up the chain of the source it remembers the mountpoint of that mount in "shorter" and the existing mount is put there for the copies.

But the loop ends at the topmost mount without ever looking at it. If the topmost mount is the only mount namespace file in the chain "shorter" stays NULL and mnt_change_mountpoint() attaches the existing mount of the copy below the root of the mount namespace file. That's a dentry of nsfs. No path walk in the copy's mount namespace ever gets there. The mount is gone until its parent goes and the copy that took its place can't be unmounted synchronously because it has a child:

  S      bind mount of a file
  N      bind mount of a mount namespace file on top of S
  A      shared mount, B a slave of A, Q mounted on B/file

move_mount(S, A/file)
cat B/file -> the content of S instead of Q
umount B/file -> EBUSY

Look at every mount of the chain including the topmost one.

---
 fs/namespace.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

@@ -2602,9 +2602,11 @@ static int attach_recursive_mnt(struct m
 	 * Preallocate a mountpoint in case the new mounts need to be
 	 * mounted beneath mounts on the same mountpoint.
 	 */
-	for (top = source_mnt; unlikely(top->overmount); top = top->overmount) {
+	for (top = source_mnt; ; top = top->overmount) {
 		if (!shorter && is_mnt_ns_file(top->mnt.mnt_root))
 			shorter = top->mnt_mp;
+		if (likely(!top->overmount))
+			break;
 	}
 	err = get_mountpoint(top->mnt.mnt_root, &root);
 	if (err)

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit d791606bb915a8f27e36657696fd1d9a216c5c08 upstream.

Commit 02587a4af82a ("fs: refuse fspick() on internal superblocks") blocked fspick() on SB_NOUSER superblocks. But that's not the only way to reconfigure_super(). mount(MS_REMOUNT) and umount() of the caller's root without MNT_DETACH. The root of an empty mount namespace is a nullfs mount and all mount namespaces share that superblock:

nullfs: fspick: FAIL errno=22 (Invalid argument)
nullfs: mount(MS_REMOUNT|MS_RDONLY): ok(0)
nullfs after remount: statfs(/): magic=0x4e554c4c flags=0x21 RDONLY
nullfs: umount2("/", 0): ok(0)

Refuse both like fspick() does. Only root in the initial user namespace can do this and nullfs is empty and immutable so the flags don't buy anything. But they show up in statfs() for the root of every mount namespace on the host.

---
 fs/fs_context.c | 4 ++++
 fs/fsopen.c     | 3 ---
 2 files changed, 4 insertions(+), 3 deletions(-)

index 23ad66cd94e1..a50584df97be 100644
@@ -315,6 +315,10 @@ struct fs_context *fs_context_for_reconfigure(struct dentry *dentry,
 					unsigned int sb_flags,
 					unsigned int sb_flags_mask)
 {
+	/* kernel-internal superblocks are nobody's to reconfigure */
+	if (dentry->d_sb->s_flags & SB_NOUSER)
+		return ERR_PTR(-EINVAL);
+
 	return alloc_fs_context(dentry->d_sb->s_type, dentry, sb_flags,
 				sb_flags_mask, FS_CONTEXT_FOR_RECONFIGURE);
 }

index 9d5a7a22b529..ae19e5136598 100644
@@ -190,9 +190,6 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
 	ret = -EINVAL;
 	if (target.mnt->mnt_root != target.dentry)
 		goto err_path;
-	/* kernel-internal superblocks are nobody's to reconfigure */
-	if (target.dentry->d_sb->s_flags & SB_NOUSER)
-		goto err_path;
 
 	fc = fs_context_for_reconfigure(target.dentry, 0, 0);
 	if (IS_ERR(fc)) {

2.56.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 1a116222acdcc1bc1a8350727ae6f28907d44594 upstream.

mnt_ns_release() drops the last passive reference of a mount namespace and removes its fanotify marks via fsnotify_mntns_delete(). That takes the mutex of every group with a mark on the namespace and the spinlock of the connector. Fine from process context. But mnt_ns_tree_remove() hands the reference the namespace was allocated with to call_rcu() and so the marks are removed from the RCU softirq:

  BUG: sleeping function called from invalid context at kernel/locking/mutex.c:623
  in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name: swapper/3
   __mutex_lock+0x113/0x24b0
   fsnotify_destroy_marks+0x11b/0x3d0
   mnt_ns_release_rcu+0x57/0xa0
   rcu_core+0x6b6/0x1e40

and lockdep complains about the connector lock being taken from softirq context. A fanotify group with a FAN_MARK_MNTNS mark on the mount namespace of another task is all that's needed. Root in a user namespace can do that for a mount namespace it owns. The task exits and the namespace is freed with the mark still on it.

Remove the marks in free_mnt_ns() before the namespace is handed to RCU. That runs in process context once the last active reference is gone. A mark is added through a file descriptor to the namespace which holds an active reference so no mark can show up after that.

---
 fs/namespace.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

@@ -130,7 +130,6 @@ static void mnt_ns_release(struct mnt_na
 {
 	/* keep alive for {list,stat}mount() */
 	if (ns && refcount_dec_and_test(&ns->passive)) {
-		fsnotify_mntns_delete(ns);
 		put_user_ns(ns->user_ns);
 		kfree(ns);
 	}

@@ -4257,6 +4256,8 @@ static void free_mnt_ns(struct mnt_names
 	if (!is_anon_ns(ns))
 		ns_common_free(ns);
 	dec_mnt_namespaces(ns->ucounts);
+	/* the last active reference is gone, no mark can show up anymore */
+	fsnotify_mntns_delete(ns);
 	mnt_ns_tree_remove(ns);
 }
 

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 6b7c001eb5857fa09041f47673666060a2e5fc1c upstream.

path_pivot_root() reads the parents of new_root and of the caller's root and checks whether they are shared before it checks that either mount is in the caller's mount namespace. Only namespace_sem is held. That's fine for a mount that is in the caller's mount namespace.

But new_root can be a file descriptor to a mount that has been unmounted and that only the file descriptor keeps alive. If that mount stayed attached to its parent when it was unmounted nothing pins the parent for it. The final mntput() of the parent unhooks the children under mount_lock alone and frees the parent afterwards:

pivot_root() close(fd), last ref on the parent
------------ ---------------------------------
ex_parent = new_mnt->mnt_parent

                                   mntput_no_expire_slowpath()
                                     __umount_mnt(new_mnt)
                                   cleanup_mnt()
                                     call_rcu()

IS_MNT_SHARED(ex_parent)

  BUG: KASAN: slab-use-after-free in path_pivot_root+0xf1a/0x1840
  Read of size 4 at addr ffff8881047382f8 by task pivot_widen/157
   path_pivot_root+0xf1a/0x1840
   __x64_sys_pivot_root+0x165/0x190

The same goes for the caller's root via chroot(). Both outcomes of the check end in EINVAL so nothing but the read itself goes wrong. It's the same thing commit bb4609405752 ("statmount: read the parent of an unmounted mount under mount_lock") fixed for statmount().

Check that both mounts are in the caller's namespace before their parents are read. Every path returns EINVAL either way.

---
 fs/namespace.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

@@ -4759,14 +4759,15 @@ int path_pivot_root(struct path *new, st
 
 	new_mnt = real_mount(new->mnt);
 	root_mnt = real_mount(root.mnt);
+	/* only a mounted mount has a parent that namespace_sem pins */
+	if (!check_mnt(root_mnt) || !check_mnt(new_mnt))
+		return -EINVAL;
 	ex_parent = new_mnt->mnt_parent;
 	root_parent = root_mnt->mnt_parent;
 	if (IS_MNT_SHARED(old_mnt) ||
 		IS_MNT_SHARED(ex_parent) ||
 		IS_MNT_SHARED(root_parent))
 		return -EINVAL;
-	if (!check_mnt(root_mnt) || !check_mnt(new_mnt))
-		return -EINVAL;
 	if (new_mnt->mnt.mnt_flags & MNT_LOCKED)
 		return -EINVAL;
 	if (d_unlinked(new->dentry))

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 75f4197f1e11d2eb75c2d6f916a7d4764db621e5 upstream.

rmdir(), unlink() and rename() call dont_mount() on the victim and then detach_mounts() with the victim's inode locked. do_lock_mount() takes the inode lock of the mountpoint and checks cant_mount() so a mount can't show up after detach_mounts().

But attach_recursive_mnt() makes a second mountpoint for the root of the source mount so that the mounts already located at the destination can be put on top of it. No inode is locked for that one and d_set_mounted() only refuses a dentry that is unlinked. Between detach_mounts() and d_delete() the victim is still hashed:

rmrace: b passed dont_mount() and detach_mounts(), sleeping
T2: move_mount(S, /tmp/plcant/x, BENEATH) = 0 errno 0 ()
T1: rmdir(/tmp/plcant/d/b) = 0 errno 0

      109 107 0:61 /d/b//deleted /tmp/plcant/x rw,relatime - tmpfs tmpfs rw
      108 109 0:63 / /tmp/plcant/x rw,relatime - tmpfs T rw

A bind mount of the directory that's being removed is moved beneath an existing mount while the rmdir() is located between the two calls. The mount ends up on the removed directory and nothing will ever detach it.

Check cant_mount() in d_set_mounted() as well. dont_mount() raises the flag under d_lock before detach_mounts() runs so either the mountpoint is set first and detach_mounts() finds the mount or the flag is seen and the mount is refused.

---
 fs/dcache.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

@@ -1583,6 +1583,8 @@ EXPORT_SYMBOL(path_has_submounts);
  *
  * Only one of d_invalidate() and d_set_mounted() must succeed.  For
  * this reason take rename_lock and d_lock on dentry and ancestors.
+ * Likewise for dont_mount() which marks a dentry that is being removed
+ * under d_lock.
  */
 int d_set_mounted(struct dentry *dentry)
 {

@@ -1599,7 +1601,7 @@ int d_set_mounted(struct dentry *dentry)
 		spin_unlock(&p->d_lock);
 	}
 	spin_lock(&dentry->d_lock);
-	if (!d_unlinked(dentry)) {
+	if (!d_unlinked(dentry) && !cant_mount(dentry)) {
 		ret = -EBUSY;
 		if (!d_mountpoint(dentry)) {
 			dentry->d_flags |= DCACHE_MOUNTED;

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 0b6f31a3c0b16066f812e94b52a704174a6f23cd upstream.

Active references on the namespaces of an nsproxy are taken when the nsproxy is installed into a task in switch_task_namespaces() and copy_namespaces() and dropped again by put_nsproxy() through deactivate_nsproxy().

But ksys_unshare() calls put_nsproxy() on an nsproxy that was never installed when set_cred_ucounts() fails. The new namespaces of that nsproxy go from zero to minus one and the namespaces shared with the caller lose a reference that belongs to the nsproxy the caller keeps using:

  WARNING: kernel/nscommon.c:171 at __ns_ref_active_put+0x1cd/0x230
   nsproxy_ns_active_put
   deactivate_nsproxy
   ksys_unshare

Afterwards the namespaces the caller lives in aren't listed by listns() anymore. set_cred_ucounts() only fails when alloc_ucounts() can't allocate and it only allocates when the real uid of the caller differs from its effective uid.

Commit cefd55bd2159 ("nsproxy: fix free_nsproxy() and simplify create_new_namespaces()") separated the two cases on purpose. nsproxy_free() frees an nsproxy that was prepared but never installed and that's what a failed setns() uses in put_nsset(). Export it and use it for a failed unshare() as well.

---
 include/linux/nsproxy.h |    1 +
 kernel/fork.c           |    3 ++-
 kernel/nsproxy.c        |    2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

@@ -100,6 +100,7 @@ void exit_cred_namespaces(struct task_st
 void switch_task_namespaces(struct task_struct *tsk, struct nsproxy *new);
 int exec_task_namespaces(void);
 void deactivate_nsproxy(struct nsproxy *ns);
+void nsproxy_free(struct nsproxy *ns);
 int unshare_nsproxy_namespaces(unsigned long, struct nsproxy **,
 	struct cred *, struct fs_struct *);
 int __init nsproxy_cache_init(void);

@@ -3326,8 +3326,9 @@ int ksys_unshare(unsigned long unshare_f
 	perf_event_namespaces(current);
 
 bad_unshare_cleanup_nsproxy:
+	/* never installed, so no active references to drop */
 	if (new_nsproxy)
-		put_nsproxy(new_nsproxy);
+		nsproxy_free(new_nsproxy);
 bad_unshare_cleanup_cred:
 	if (new_cred)
 		put_cred(new_cred);

@@ -61,7 +61,7 @@ static inline struct nsproxy *create_nsp
 	return nsproxy;
 }
 
-static inline void nsproxy_free(struct nsproxy *ns)
+void nsproxy_free(struct nsproxy *ns)
 {
 	put_mnt_ns(ns->mnt_ns);
 	put_uts_ns(ns->uts_ns);

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit b138e16fb1e81376cea89f727dda7bfccd140de4 upstream.

d_alloc_pseudo() hands out dentries for pipes, sockets and other files that are never anyone's child or parent. They carry DCACHE_NORCU and dentry_free() frees them right away because no lockless path walk can ever reach them. That holds as long as such a dentry isn't the root of a mount. But bind mounting /proc/self/fd/<fd> of such a file does exactly that.

Most callers of alloc_file_pseudo() put their files on kernel-internal mounts and may_copy_tree() refuses those. bpf_token_create() doesn't. It places the token file on the bpffs mount the caller handed it and that mount is in the caller's mount namespace so the clone goes through:

  mount --bind /proc/self/fd/<token> <file>
  open_tree(tokfd, "", AT_EMPTY_PATH | OPEN_TREE_CLONE) + move_mount()

__follow_mount_rcu() then loads ->mnt_root of that mount, reads d_seq and d_flags of the dentry and only then checks mount_lock. The dentry can be gone by then. The final mntput() of an unmounted parent unhooks a child that stayed attached to it under mount_lock alone. When the child's holder does the final put right after that the root is dput() and freed immediately while a walker that found the mount hashed still looks at it.

Refuse to clone a mount with a DCACHE_NORCU dentry as its root. Nothing sensible can be done with a bind mount of a bpf token anyway.

---
 fs/namespace.c |    4 ++++
 1 file changed, 4 insertions(+)

@@ -3008,6 +3008,10 @@ static struct mount *__do_loopback(const
 	if (!may_copy_tree(old_path))
 		return ERR_PTR(-EINVAL);
 
+	/* a pseudo dentry is freed without an RCU delay, no walk may find it */
+	if (old_path->dentry->d_flags & DCACHE_NORCU)
+		return ERR_PTR(-EINVAL);
+
 	if (recurse && !old->mnt_ns)
 		return ERR_PTR(-EINVAL);
 

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 528b383dc35d1e5caf64ddf5d35f983ae543b0d0 upstream.

unlink(), rmdir() and rename() remove the entry from the filesystem, call detach_mounts() on the entry with the inode locked and then call d_delete() once the inode is unlocked.

The thing is that between detach_mounts() and d_delete() the dentry is still hashed and positive. But after detach_mounts() nothing covers the dentry anymore. A lookup that finds this dentry in the dcache can uncover what the mounts hid.

That's a problem when unlinking files or directories that are mountpoints in other mount namespaces. Everybody who had the underlying entry covered can race the detach_mount() call until d_delete() has run.

The race window isn't all that small. It encompasses namespace_unlock() with a full synchronize_rcu_expedited() grace period and the inode_unlock() of the entry.

In my experiments three walkers that kept trying read an overmounted file in 11938 of the 16140 unlinks that removed its mountpoint from a bind mount of the filesystem within a minute.

Fun fact, d_invalidate() has the same ordering problem but gets it right. It unhashes the dentry first and detaches the mounts afterwards. Let's do the same in __detach_mounts():

- A lookup that hasn't found the dentry yet misses it in the dcache and waits for the directory lock that the caller holds until the name is gone for good.

- A lockless lookup that found it already fails the mount_lock check while the dentry still counts as a mountpoint and the d_seq check once it doesn't, and retries.

This fixes the lockless path. We still need to fix the reference count lookup in a follow-up patch.

So d_drop() the dentry. The dentry stays positive and held but can't be found anymore. Then proceed with the detach and unlink.

Reproducer:

The reads were counted with the program below, three walkers for 60 s in a VM with 4 CPUs. The race window is stretched with the debug patch pasted here. fs.detach_race_walk_us sleeps in lookup_fast() once it found a mountpoint dentry and before its mounts are crossed. fs.detach_race_unlink_us sleeps in vfs_unlink() between detach_mounts() and d_delete().

  // SPDX-License-Identifier: GPL-2.0
  /*
   * unlink_covered: unlink a file that is a mountpoint in a detached copy of
   * its mount, against lookups that open it through that copy.
   *
   * A is a tmpfs with the file f1 (content SECRET_F) and the plain file
   * MARK_A. A' is an open_tree(OPEN_TREE_CLONE) copy of A with MARK_A bound
   * on A'/f1, held through an O_PATH fd on its root once the tree fd is
   * closed. Walkers open f1 through that fd while the driver unlinks A/f1,
   * where nothing is mounted on it. A walker may read MARK_A or get ENOENT.
   * A read of SECRET_F is a hit: the name was found after its mount was
   * gone. The lockless walks use openat2(RESOLVE_CACHED).
   *
   * usage: unlink_covered [-t seconds] [-w walkers]
   */
  #ifndef _GNU_SOURCE
  #define _GNU_SOURCE
  #endif
  #include <errno.h>
  #include <fcntl.h>
  #include <pthread.h>
  #include <stdatomic.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <string.h>
  #include <unistd.h>
  #include <sys/mount.h>
  #include <sys/stat.h>
  #include <sys/syscall.h>
  #include <linux/openat2.h>

  #ifndef __NR_open_tree
  #define __NR_open_tree 428
  #endif
  #ifndef __NR_move_mount
  #define __NR_move_mount 429
  #endif
  #ifndef __NR_openat2
  #define __NR_openat2 437
  #endif
  #ifndef OPEN_TREE_CLONE
  #define OPEN_TREE_CLONE 1
  #endif
  #ifndef OPEN_TREE_CLOEXEC
  #define OPEN_TREE_CLOEXEC O_CLOEXEC
  #endif
  #ifndef MOVE_MOUNT_F_EMPTY_PATH
  #define MOVE_MOUNT_F_EMPTY_PATH 0x00000004
  #endif

  #define WORK "/tmp/uc"
  #define ADIR WORK "/A"

static int duration = 60, nwalkers = 3;
static atomic_int stop, writer_waiting;
static pthread_rwlock_t cur_lock = PTHREAD_RWLOCK_INITIALIZER;
static int cur_fd = -1; /* the root of A', -1 while there is none */
static atomic_long n_unlink, n_walk, n_mark, n_enoent, n_other, n_secret,
n_secret_cached;

  static void die(const char *what)
  {
  	fprintf(stderr, "FATAL %s: %s\n", what, strerror(errno));
  	exit(2);
  }

static void put_file(int dfd, const char *name, const char *content) {
int fd = openat(dfd, name, O_CREAT | O_WRONLY | O_TRUNC | O_CLOEXEC,
0644);

  	if (fd < 0 || write(fd, content, strlen(content)) < 0)
  		die(name);
  	close(fd);
  }

  /* "plain/../" @n times, then f1: a longer walk that checks nothing on its way */
  static char *longpath(int n)
  {
  	char *p = malloc(n * 9 + 3), *q = p;

  	for (int i = 0; i < n; i++, q += 9)
  		memcpy(q, "plain/../", 9);
  	strcpy(q, "f1");
  	return p;
  }

  static void try_read(int dfd, const char *path, int cached)
  {
  	struct open_how how = { .flags = O_RDONLY | O_CLOEXEC,
  				.resolve = RESOLVE_CACHED };
  	char buf[32] = "";
  	long n;
  	int fd;

  	if (cached)
  		fd = syscall(__NR_openat2, dfd, path, &how, sizeof(how));
  	else
  		fd = openat(dfd, path, O_RDONLY | O_CLOEXEC);
  	atomic_fetch_add(&n_walk, 1);
  	if (fd < 0) {
  		if (errno == ENOENT)
  			atomic_fetch_add(&n_enoent, 1);
  		else if (!cached || errno != EAGAIN)
  			atomic_fetch_add(&n_other, 1);
  		return;
  	}
  	n = read(fd, buf, sizeof(buf) - 1);
  	close(fd);
  	if (n >= 6 && !strncmp(buf, "SECRET", 6)) {
  		if (cached)
  			atomic_fetch_add(&n_secret_cached, 1);
  		if (!atomic_fetch_add(&n_secret, 1))
  			printf("HIT: read \"%s\" through %s (%s walk)\n", buf,
  			       path, cached ? "lockless" : "any");
  	} else {
  		atomic_fetch_add(&n_mark, 1);
  	}
  }

  static void *walker(void *arg)
  {
  	unsigned int r = (long)arg * 2654435761u;

  	while (!atomic_load(&stop)) {
  		char *p_long, *p_short;
  		int dfd;

  		while (atomic_load(&writer_waiting) && !atomic_load(&stop))
  			usleep(20);
  		pthread_rwlock_rdlock(&cur_lock);
  		dfd = cur_fd;
  		if (dfd < 0) {
  			pthread_rwlock_unlock(&cur_lock);
  			usleep(100);
  			continue;
  		}
  		r = r * 1103515245u + 12345u;
  		p_long = longpath(1 + (r >> 8) % 400);
  		p_short = longpath(0);
  		try_read(dfd, p_long, 0);
  		try_read(dfd, p_short, 0);
  		try_read(dfd, p_long, 1);
  		pthread_rwlock_unlock(&cur_lock);
  		free(p_long);
  		free(p_short);
  	}
  	return NULL;
  }

/* hand the walkers a new A' (or none), close the old one */ static void publish(int fd)
{
int old;

  	atomic_fetch_add(&writer_waiting, 1);
  	pthread_rwlock_wrlock(&cur_lock);
  	old = cur_fd;
  	cur_fd = fd;
  	pthread_rwlock_unlock(&cur_lock);
  	atomic_fetch_sub(&writer_waiting, 1);
  	if (old >= 0)
  		close(old);
  }

  static void *driver(void *arg __attribute__((unused)))
  {
  	int a;

  	if (mkdir(ADIR, 0755) && errno != EEXIST)
  		die("mkdir A");
  	if (mount("A", ADIR, "tmpfs", 0, "size=4M"))
  		die("mount A");
  	a = open(ADIR, O_PATH | O_DIRECTORY | O_CLOEXEC);
  	if (a < 0 || mkdirat(a, "plain", 0755))
  		die("A/plain");
  	put_file(a, "MARK_A", "MARK_A");
  	while (!atomic_load(&stop)) {
  		int t, m, fd_a;

  		put_file(a, "f1", "SECRET_F");
  		t = syscall(__NR_open_tree, a, "",
  			    OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC | AT_EMPTY_PATH);
  		if (t < 0)
  			die("open_tree A");
  		m = syscall(__NR_open_tree, a, "MARK_A",
  			    OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC);
  		if (m < 0)
  			die("open_tree MARK_A");
  		if (syscall(__NR_move_mount, m, "", t, "f1",
  			    MOVE_MOUNT_F_EMPTY_PATH))
  			die("move_mount");
  		close(m);
  		fd_a = openat(t, ".", O_PATH | O_DIRECTORY | O_CLOEXEC);
  		if (fd_a < 0)
  			die("open A'");
  		publish(fd_a);
  		usleep(200 + rand() % 1000);
  		close(t);			/* A' is unmounted, fd_a holds it */
  		usleep(200 + rand() % 1000);
  		if (unlinkat(a, "f1", 0))	/* through A, a plain file there */
  			die("unlink f1");
  		atomic_fetch_add(&n_unlink, 1);
  		usleep(rand() % 300);
  		publish(-1);
  	}
  	close(a);
  	umount2(ADIR, MNT_DETACH);
  	return NULL;
  }

  int main(int argc, char **argv)
  {
  	pthread_t d, *w;
  	int c, i;

  	setvbuf(stdout, NULL, _IOLBF, 0);
  	while ((c = getopt(argc, argv, "t:w:")) != -1) {
  		switch (c) {
  		case 't':
  			duration = atoi(optarg);
  			break;
  		case 'w':
  			nwalkers = atoi(optarg);
  			break;
  		default:
  			fprintf(stderr, "usage: unlink_covered [-t seconds] [-w walkers]\n");
  			return 2;
  		}
  	}
  	if (unshare(CLONE_NEWNS) ||
  	    mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL))
  		die("unshare");
  	if (mkdir(WORK, 0755) && errno != EEXIST)
  		die("mkdir");
  	w = calloc(nwalkers, sizeof(*w));
  	for (i = 0; i < nwalkers; i++)
  		if (pthread_create(&w[i], NULL, walker, (void *)(long)i))
  			die("pthread_create");
  	if (pthread_create(&d, NULL, driver, NULL))
  		die("pthread_create");
  	sleep(duration);
  	atomic_store(&stop, 1);
  	pthread_join(d, NULL);
  	for (i = 0; i < nwalkers; i++)
  		pthread_join(w[i], NULL);
  	printf("unlink_covered: %d s, %d walkers: unlinks %ld walks %ld mark %ld enoent %ld other %ld SECRET %ld (lockless %ld)\n",
  	       duration, nwalkers, atomic_load(&n_unlink), atomic_load(&n_walk),
  	       atomic_load(&n_mark), atomic_load(&n_enoent),
  	       atomic_load(&n_other), atomic_load(&n_secret),
  	       atomic_load(&n_secret_cached));
  	return atomic_load(&n_secret) ? 1 : 0;
  }

#  diff --git a/fs/namei.c b/fs/namei.c
#  --- a/fs/namei.c
#  +++ b/fs/namei.c
#  @@ -35,6 +35,7 @@
#   #include <linux/fcntl.h>
#   #include <linux/device_cgroup.h>
#   #include <linux/fs_struct.h>
#  +#include <linux/delay.h>
#   #include <linux/posix_acl.h>
#   #include <linux/hash.h>
#   #include <linux/bitops.h>
#  @@ -1205,9 +1206,26 @@ static int sysctl_protected_symlinks __read_mostly;
#   static int sysctl_protected_hardlinks __read_mostly;
#   static int sysctl_protected_fifos __read_mostly;
#   static int sysctl_protected_regular __read_mostly;
#  +/* debug: widen the two windows of the detach_mounts() race */
#  +static int sysctl_detach_race_walk_us __read_mostly;
#  +static int sysctl_detach_race_unlink_us __read_mostly;
#
#   #ifdef CONFIG_SYSCTL
#   static const struct ctl_table namei_sysctls[] = {
#  +	{
#  +		.procname	= "detach_race_walk_us",
#  +		.data		= &sysctl_detach_race_walk_us,
#  +		.maxlen		= sizeof(int),
#  +		.mode		= 0644,
#  +		.proc_handler	= proc_dointvec,
#  +	},
#  +	{
#  +		.procname	= "detach_race_unlink_us",
#  +		.data		= &sysctl_detach_race_unlink_us,
#  +		.maxlen		= sizeof(int),
#  +		.mode		= 0644,
#  +		.proc_handler	= proc_dointvec,
#  +	},
#   	{
#   		.procname	= "protected_symlinks",
#   		.data		= &sysctl_protected_symlinks,
#  @@ -1878,6 +1896,10 @@ static struct dentry *lookup_fast(struct nameidata *nd)
#   		dentry = __d_lookup(parent, &nd->last);
#   		if (unlikely(!dentry))
#   			return NULL;
#  +		/* debug: between finding a mountpoint and crossing its mounts */
#  +		if (unlikely(sysctl_detach_race_walk_us) && d_mountpoint(dentry))
#  +			usleep_range(sysctl_detach_race_walk_us,
#  +				     sysctl_detach_race_walk_us + 10);
#   		status = d_revalidate(nd->inode, &nd->last, dentry, nd->flags);
#   	}
#   	if (unlikely(status <= 0)) {
#  @@ -5693,6 +5715,10 @@ int vfs_unlink(struct mnt_idmap *idmap, struct inode *dir,
#   			if (!error) {
#   				dont_mount(dentry);
#   				detach_mounts(dentry);
#  +				/* debug: the mounts are gone, d_delete() is still to come */
#  +				if (unlikely(sysctl_detach_race_unlink_us))
#  +					usleep_range(sysctl_detach_race_unlink_us,
#  +						     sysctl_detach_race_unlink_us + 10);
#   			}
#   		}
#   	}

---
 fs/namespace.c |    5 +++++
 1 file changed, 5 insertions(+)

@@ -1984,6 +1984,9 @@ out:
  * detach_mounts allows lazily unmounting those mounts instead of
  * leaking them.
  *
+ * The dentry is unhashed before the mounts go so that no lookup finds
+ * what they covered. The caller removes it for good afterwards.
+ *
  * The caller may hold dentry->d_inode->i_rwsem.
  */
 void __detach_mounts(struct dentry *dentry)

@@ -1997,6 +2000,8 @@ void __detach_mounts(struct dentry *dent
 	if (!lookup_mountpoint(dentry, &mp))
 		return;
 
+	/* the name goes first, what covered it goes second */
+	d_drop(dentry);
 	event++;
 	while (mp.node.next) {
 		mnt = hlist_entry(mp.node.next, struct mount, mnt_mp_list);

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 8189ab688f5a2c745dc90b63374da02f213d6994 upstream.

In rcuwalk the dentry is validated before it is accepted. step_into() step_into() rechecks d_seq and __follow_mount_rcu() rechecks mount_lock. An entry that gets unlinked in between causes the lookup to retry and miss.

A refwalk doesn't do this. lookup_fast() takes a reference on the hashed dentry and simply accepts it. So an unlink that happens after the reference was taken isn't seen by refwalk. That's fine for a simple file. We just happened to open it before it was unlinked, no problem.

For a mountpoint and specifically a locked mountpoint it very much isn't. The unlink detaches all mounts and then removes the name. Any refwalk that hasn't traversed the mounts yet simply reveals the underlying entry. It's a very narrow window but it can be hit:

reads of a covered file in 60 s, 3 walkers, ~15000 unlinks
no widening 27
that step + 200 us 3741

See the appended patch for a more reliable reproducer.

So check the entry after step_into(). unlink(), rmdir() and rename() mark the dentry with dont_mount() before they detach the mounts and remove the dentry.

So a dentry that is marked with DCACHE_CANT_MOUNT and is unhashed by the time its mounts were looked at is a name that was unlinked under the refwalk. DCACHE_CANT_MOUNT is read after the mounts were looked up and it is set before they are detached by detach_mounts(). A refwalk that missed the mounts will see DCACHE_CANT_MOUNT.

Plain d_unlinked() is fine. A rename takes the dentry off its hash chain but ___d_drop() leaves d_hash.pprev set. So only __d_drop() and a rename over the dentry unhash it. The only move that flips IS_ROOT splices in a disconnected alias. That doesn't have DCACHE_CANT_MOUNT set.

Add the to step_into_slowpath(). ".." and LOOKUP_DOWN may legitimately land on an unhashed directory. A refwalk that crossed onto a mount has path.mnt different from nd->path.mnt. A dentry that a filesystem dropped on its own (d_invalidate(), d_drop()) doesn't have the flag set and is treated as before.

  // SPDX-License-Identifier: GPL-2.0
  /*
   * unlink_covered: unlink a file that is a mountpoint in a detached copy of
   * its mount, against walkers that open it through that copy.
   *
   * A is a tmpfs with the file f1 (content SECRET_F) and the plain file
   * MARK_A. A' is an open_tree(OPEN_TREE_CLONE) copy of A with MARK_A bound
   * on A'/f1, held through an O_PATH fd on its root once the tree fd is
   * closed. Walkers open f1 through that fd while the driver unlinks A/f1,
   * where nothing is mounted on it. A walker may read MARK_A or get ENOENT.
   * A read of SECRET_F is a hit: the name was found after its mount was
   * gone. The lockless walks use openat2(RESOLVE_CACHED).
   *
   * usage: unlink_covered [-t seconds] [-w walkers]
   */
  #ifndef _GNU_SOURCE
  #define _GNU_SOURCE
  #endif
  #include <errno.h>
  #include <fcntl.h>
  #include <pthread.h>
  #include <stdatomic.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <string.h>
  #include <unistd.h>
  #include <sys/mount.h>
  #include <sys/stat.h>
  #include <sys/syscall.h>
  #include <linux/openat2.h>

  #ifndef __NR_open_tree
  #define __NR_open_tree 428
  #endif
  #ifndef __NR_move_mount
  #define __NR_move_mount 429
  #endif
  #ifndef __NR_openat2
  #define __NR_openat2 437
  #endif
  #ifndef OPEN_TREE_CLONE
  #define OPEN_TREE_CLONE 1
  #endif
  #ifndef OPEN_TREE_CLOEXEC
  #define OPEN_TREE_CLOEXEC O_CLOEXEC
  #endif
  #ifndef MOVE_MOUNT_F_EMPTY_PATH
  #define MOVE_MOUNT_F_EMPTY_PATH 0x00000004
  #endif

  #define WORK "/tmp/uc"
  #define ADIR WORK "/A"

static int duration = 60, nwalkers = 3;
static atomic_int stop, writer_waiting;
static pthread_rwlock_t cur_lock = PTHREAD_RWLOCK_INITIALIZER;
static int cur_fd = -1; /* the root of A', -1 while there is none */
static atomic_long n_unlink, n_walk, n_mark, n_enoent, n_other, n_secret,
n_secret_cached;

  static void die(const char *what)
  {
  	fprintf(stderr, "FATAL %s: %s\n", what, strerror(errno));
  	exit(2);
  }

static void put_file(int dfd, const char *name, const char *content) {
int fd = openat(dfd, name, O_CREAT | O_WRONLY | O_TRUNC | O_CLOEXEC,
0644);

  	if (fd < 0 || write(fd, content, strlen(content)) < 0)
  		die(name);
  	close(fd);
  }

  /* "plain/../" @n times, then f1: a longer walk that checks nothing on its way */
  static char *longpath(int n)
  {
  	char *p = malloc(n * 9 + 3), *q = p;

  	for (int i = 0; i < n; i++, q += 9)
  		memcpy(q, "plain/../", 9);
  	strcpy(q, "f1");
  	return p;
  }

  static void try_read(int dfd, const char *path, int cached)
  {
  	struct open_how how = { .flags = O_RDONLY | O_CLOEXEC,
  				.resolve = RESOLVE_CACHED };
  	char buf[32] = "";
  	long n;
  	int fd;

  	if (cached)
  		fd = syscall(__NR_openat2, dfd, path, &how, sizeof(how));
  	else
  		fd = openat(dfd, path, O_RDONLY | O_CLOEXEC);
  	atomic_fetch_add(&n_walk, 1);
  	if (fd < 0) {
  		if (errno == ENOENT)
  			atomic_fetch_add(&n_enoent, 1);
  		else if (!cached || errno != EAGAIN)
  			atomic_fetch_add(&n_other, 1);
  		return;
  	}
  	n = read(fd, buf, sizeof(buf) - 1);
  	close(fd);
  	if (n >= 6 && !strncmp(buf, "SECRET", 6)) {
  		if (cached)
  			atomic_fetch_add(&n_secret_cached, 1);
  		if (!atomic_fetch_add(&n_secret, 1))
  			printf("HIT: read \"%s\" through %s (%s walk)\n", buf,
  			       path, cached ? "lockless" : "any");
  	} else {
  		atomic_fetch_add(&n_mark, 1);
  	}
  }

  static void *walker(void *arg)
  {
  	unsigned int r = (long)arg * 2654435761u;

  	while (!atomic_load(&stop)) {
  		char *p_long, *p_short;
  		int dfd;

  		while (atomic_load(&writer_waiting) && !atomic_load(&stop))
  			usleep(20);
  		pthread_rwlock_rdlock(&cur_lock);
  		dfd = cur_fd;
  		if (dfd < 0) {
  			pthread_rwlock_unlock(&cur_lock);
  			usleep(100);
  			continue;
  		}
  		r = r * 1103515245u + 12345u;
  		p_long = longpath(1 + (r >> 8) % 400);
  		p_short = longpath(0);
  		try_read(dfd, p_long, 0);
  		try_read(dfd, p_short, 0);
  		try_read(dfd, p_long, 1);
  		pthread_rwlock_unlock(&cur_lock);
  		free(p_long);
  		free(p_short);
  	}
  	return NULL;
  }

/* hand the walkers a new A' (or none), close the old one */ static void publish(int fd)
{
int old;

  	atomic_fetch_add(&writer_waiting, 1);
  	pthread_rwlock_wrlock(&cur_lock);
  	old = cur_fd;
  	cur_fd = fd;
  	pthread_rwlock_unlock(&cur_lock);
  	atomic_fetch_sub(&writer_waiting, 1);
  	if (old >= 0)
  		close(old);
  }

  static void *driver(void *arg __attribute__((unused)))
  {
  	int a;

  	if (mkdir(ADIR, 0755) && errno != EEXIST)
  		die("mkdir A");
  	if (mount("A", ADIR, "tmpfs", 0, "size=4M"))
  		die("mount A");
  	a = open(ADIR, O_PATH | O_DIRECTORY | O_CLOEXEC);
  	if (a < 0 || mkdirat(a, "plain", 0755))
  		die("A/plain");
  	put_file(a, "MARK_A", "MARK_A");
  	while (!atomic_load(&stop)) {
  		int t, m, fd_a;

  		put_file(a, "f1", "SECRET_F");
  		t = syscall(__NR_open_tree, a, "",
  			    OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC | AT_EMPTY_PATH);
  		if (t < 0)
  			die("open_tree A");
  		m = syscall(__NR_open_tree, a, "MARK_A",
  			    OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC);
  		if (m < 0)
  			die("open_tree MARK_A");
  		if (syscall(__NR_move_mount, m, "", t, "f1",
  			    MOVE_MOUNT_F_EMPTY_PATH))
  			die("move_mount");
  		close(m);
  		fd_a = openat(t, ".", O_PATH | O_DIRECTORY | O_CLOEXEC);
  		if (fd_a < 0)
  			die("open A'");
  		publish(fd_a);
  		usleep(200 + rand() % 1000);
  		close(t);			/* A' is unmounted, fd_a holds it */
  		usleep(200 + rand() % 1000);
  		if (unlinkat(a, "f1", 0))	/* through A, a plain file there */
  			die("unlink f1");
  		atomic_fetch_add(&n_unlink, 1);
  		usleep(rand() % 300);
  		publish(-1);
  	}
  	close(a);
  	umount2(ADIR, MNT_DETACH);
  	return NULL;
  }

  int main(int argc, char **argv)
  {
  	pthread_t d, *w;
  	int c, i;

  	setvbuf(stdout, NULL, _IOLBF, 0);
  	while ((c = getopt(argc, argv, "t:w:")) != -1) {
  		switch (c) {
  		case 't':
  			duration = atoi(optarg);
  			break;
  		case 'w':
  			nwalkers = atoi(optarg);
  			break;
  		default:
  			fprintf(stderr, "usage: unlink_covered [-t seconds] [-w walkers]\n");
  			return 2;
  		}
  	}
  	if (unshare(CLONE_NEWNS) ||
  	    mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL))
  		die("unshare");
  	if (mkdir(WORK, 0755) && errno != EEXIST)
  		die("mkdir");
  	w = calloc(nwalkers, sizeof(*w));
  	for (i = 0; i < nwalkers; i++)
  		if (pthread_create(&w[i], NULL, walker, (void *)(long)i))
  			die("pthread_create");
  	if (pthread_create(&d, NULL, driver, NULL))
  		die("pthread_create");
  	sleep(duration);
  	atomic_store(&stop, 1);
  	pthread_join(d, NULL);
  	for (i = 0; i < nwalkers; i++)
  		pthread_join(w[i], NULL);
  	printf("unlink_covered: %d s, %d walkers: unlinks %ld walks %ld mark %ld enoent %ld other %ld SECRET %ld (lockless %ld)\n",
  	       duration, nwalkers, atomic_load(&n_unlink), atomic_load(&n_walk),
  	       atomic_load(&n_mark), atomic_load(&n_enoent),
  	       atomic_load(&n_other), atomic_load(&n_secret),
  	       atomic_load(&n_secret_cached));
  	return atomic_load(&n_secret) ? 1 : 0;
  }

#  diff --git a/fs/namei.c b/fs/namei.c
#  --- a/fs/namei.c
#  +++ b/fs/namei.c
#  @@ -35,6 +35,7 @@
#   #include <linux/fcntl.h>
#   #include <linux/device_cgroup.h>
#   #include <linux/fs_struct.h>
#  +#include <linux/delay.h>
#   #include <linux/posix_acl.h>
#   #include <linux/hash.h>
#   #include <linux/bitops.h>
#  @@ -1205,9 +1206,26 @@ static int sysctl_protected_symlinks __read_mostly;
#   static int sysctl_protected_hardlinks __read_mostly;
#   static int sysctl_protected_fifos __read_mostly;
#   static int sysctl_protected_regular __read_mostly;
#  +/* debug: widen the two windows of the detach_mounts() race */
#  +static int sysctl_detach_race_walk_us __read_mostly;
#  +static int sysctl_detach_race_unlink_us __read_mostly;
#
#   #ifdef CONFIG_SYSCTL
#   static const struct ctl_table namei_sysctls[] = {
#  +	{
#  +		.procname	= "detach_race_walk_us",
#  +		.data		= &sysctl_detach_race_walk_us,
#  +		.maxlen		= sizeof(int),
#  +		.mode		= 0644,
#  +		.proc_handler	= proc_dointvec,
#  +	},
#  +	{
#  +		.procname	= "detach_race_unlink_us",
#  +		.data		= &sysctl_detach_race_unlink_us,
#  +		.maxlen		= sizeof(int),
#  +		.mode		= 0644,
#  +		.proc_handler	= proc_dointvec,
#  +	},
#   	{
#   		.procname	= "protected_symlinks",
#   		.data		= &sysctl_protected_symlinks,
#  @@ -1878,6 +1896,10 @@ static struct dentry *lookup_fast(struct nameidata *nd)
#   		dentry = __d_lookup(parent, &nd->last);
#   		if (unlikely(!dentry))
#   			return NULL;
#  +		/* debug: between finding a mountpoint and crossing its mounts */
#  +		if (unlikely(sysctl_detach_race_walk_us) && d_mountpoint(dentry))
#  +			usleep_range(sysctl_detach_race_walk_us,
#  +				     sysctl_detach_race_walk_us + 10);
#   		status = d_revalidate(nd->inode, &nd->last, dentry, nd->flags);
#   	}
#   	if (unlikely(status <= 0)) {
#  @@ -5693,6 +5715,10 @@ int vfs_unlink(struct mnt_idmap *idmap, struct inode *dir,
#   			if (!error) {
#   				dont_mount(dentry);
#   				detach_mounts(dentry);
#  +				/* debug: the mounts are gone, d_delete() is still to come */
#  +				if (unlikely(sysctl_detach_race_unlink_us))
#  +					usleep_range(sysctl_detach_race_unlink_us,
#  +						     sysctl_detach_race_unlink_us + 10);
#   			}
#   		}
#   	}

---
 fs/namei.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

@@ -2087,6 +2087,33 @@ all_done: // pure jump
 }
 
 /*
+ * Be careful in case the dentry is unlinked or renamed. Any mounts
+ * stacked on top of it are going away. We need to make sure that we
+ * don't reveal the underyling dentry during refwalk. In rcuwalk we
+ * catch this via d_seq and another lookup for the name. Give the same
+ * guarantee in refwalk.
+ */
+static bool unlink_may_reveal(struct nameidata *nd, int flags,
+			      struct path *path, struct dentry *dentry)
+{
+	/* ".." and LOOKUP_DOWN may land on an unhashed directory */
+	if (flags & WALK_NOFOLLOW)
+		return false;
+	if (nd->flags & LOOKUP_REVAL)
+		return false;
+	/* We crossed onto a mount and the name led us here while it still existed */
+	if (path->mnt != nd->path.mnt)
+		return false;
+	/* only a name on its way out is flagged */
+	if (likely(!cant_mount(dentry)))
+		return false;
+	if (!d_unlinked(dentry))
+		return false;
+	dput(no_free_ptr(path->dentry));
+	return true;
+}
+
+/*
  * Do we need to follow links? We _really_ want to be able
  * to do this check without having to look at inode->i_op,
  * so we keep a cache of "no, this doesn't need follow_link"

@@ -2115,6 +2142,8 @@ static noinline const char *step_into_sl
 			if (unlikely(!inode))
 				return ERR_PTR(-ENOENT);
 		} else {
+			if (unlikely(unlink_may_reveal(nd, flags, &path, dentry)))
+				return ERR_PTR(-ESTALE);
 			dput(nd->path.dentry);
 			if (nd->path.mnt != path.mnt)
 				mntput(nd->path.mnt);

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit b0d1022b9bdb1e98aa7fb3244fdd50a58c61766c upstream.

An immutable inode is never written to so write hints are pointless. Refuse the hint on an immutable inode the way setattr(), fallocate() and etxattr() refuse their changes.

---
 fs/fcntl.c |    3 +++
 1 file changed, 3 insertions(+)

@@ -372,6 +372,9 @@ static long fcntl_set_rw_hint(struct fil
 	u64 __user *argp = (u64 __user *)arg;
 	u64 hint;
 
+	/* nothing is ever written to it */
+	if (IS_IMMUTABLE(inode))
+		return -EPERM;
 	if (!inode_owner_or_capable(file_mnt_idmap(file), inode))
 		return -EPERM;
 

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit ba985ce08471c555af2a2fbe217c1bc2e72a6d8c upstream.

It's possible to add autmounts even when the parent mount isn't in the mount namespace of the caller. The only requirement we have is that the parent mount namespace must not be NULL, i.e., unmounted.

Problem is that clone_private_mount() has MNT_NS_INTERNAL which makes that trivially true. So that passes the test and attach_recursive_mnt() accepts that as a mount point and funny enough, count_mounts() dereferences MNT_NS_INTERNAL. The problem is it is an error pointer...

So we can reach this in userspace via fanotify. A filesystem mark on the lower filesystem of an overlay reports paths on the layer clone and reading the event hands out a descriptor on it. For example with debugfs as the lower layer it goes kaboom:

openat(evfd, "tracing", O_DIRECTORY)

  Oops: general protection fault
  KASAN: null-ptr-deref in range [0x1d0-0x1d7]
  RIP: 0010:count_mounts+0x35/0x200
   attach_recursive_mnt
   finish_automount

And since that sleeping beauty happens under namespace_sem held for writing every mount operation on the system blocks from then on. Congrats.

Use is_mounted() instead which rejects unmounted and internal mounts alike. The open fails with EINVAL just as it did before clone_private_mount() used MNT_NS_INTERNAL.

---
 fs/namespace.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

@@ -3784,7 +3784,7 @@ static int do_add_mount(struct mount *ne
 		if (!(mnt_flags & MNT_SHRINKABLE))
 			return -EINVAL;
 		/* ... and for those we'd better have mountpoint still alive */
-		if (!parent->mnt_ns)
+		if (!is_mounted(&parent->mnt))
 			return -EINVAL;
 	}
 

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit caf16f9b4aa66e5092f95e98cda54f2bb7482ed2 upstream.

There's no point in updating access times of the nullfs instance. Raise S_IMMUTABLE.

---
 fs/nullfs.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

@@ -30,8 +30,8 @@ static int nullfs_fs_fill_super(struct s
 	make_empty_dir_inode(inode);
 	simple_inode_init_ts(inode);
 	inode->i_ino	= 1;
-	/* ... and immutable. */
-	inode->i_flags |= S_IMMUTABLE;
+	/* ... and immutable, reading it leaves no trace either. */
+	inode->i_flags |= S_IMMUTABLE | S_NOATIME;
 
 	s->s_root = d_make_root(inode);
 	if (!s->s_root)

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit f78ddcc872248c7aa1d7e904928441a0be4c6d5d upstream.

Locked mounts are special. They protect the underlying files and directories from being revealed. do_umount() refuses to unmount locked mounts but shrink_submounts() doesn't.

A shrinkable mount can become locked once the owner of a user namespace puts it beneath a locked mount with MOVE_MOUNT_BENEATH. The locked property now moves to the mount at the bottom making it possible to unmount the top mount.

So umount() of an unlocked ancestor now expires the bottom mount first and the covered directory is revealed:

move_mount(c -> x/hidden, MOVE_MOUNT_BENEATH) = 0

  the cover after the lock moved:  umount2(x/hidden) = 0
  the holder of the lock:          umount2(x/hidden) = EINVAL
  the root of the copy, busy:      umount2(x) = EBUSY

reads x/hidden/secret: "covered-by-root"

So leave a locked mount alone as it dies together with its parent. A plain umount() of an unlocked mount with a locked one below it is EBUSY from now on. It's the same for any other locked child. A lazy umount still takes the whole tree.

mark_mounts_for_expiry() never sees a locked mount. lock_mnt_tree() leaves a mount on an expiry list alone and nothing else puts a locked one on such a list. Add an assert for this.

---
 fs/namespace.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

@@ -3994,6 +3994,8 @@ void mark_mounts_for_expiry(struct list_
 	list_for_each_entry_safe(mnt, next, mounts, mnt_expire) {
 		if (!is_mounted(&mnt->mnt))
 			continue;
+		/* lock_mnt_tree() leaves expirable mounts alone */
+		VFS_WARN_ON_ONCE(IS_MNT_LOCKED(mnt));
 		if (!xchg(&mnt->mnt_expiry_mark, 1) ||
 			propagate_mount_busy(mnt, 1))
 			continue;

@@ -4020,7 +4022,8 @@ EXPORT_SYMBOL_GPL(mark_mounts_for_expiry
  */
 static bool shrink_submount(struct mount *mnt)
 {
-	if (propagate_mount_busy(mnt, 1))
+	/* not the kernel's to remove either */
+	if (IS_MNT_LOCKED(mnt) || propagate_mount_busy(mnt, 1))
 		return false;
 	touch_mnt_namespace(mnt->mnt_ns);
 	umount_tree(mnt, UMOUNT_PROPAGATE|UMOUNT_SYNC);

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit 33aadc7c72055fca93cb5a3691d836ebf8b31c9b upstream.

If mounts are propagated across user namespaces, attach_recursive_mnt() locks every copy of the source mount to protect overmounts from vanishing and revealing the underlying files or directories.

The user namespace is taken from the caller's mount namespaces since this is where the mounts end up. Except, that's not always true. Automounts may legitimately get popped in by tasks located in a different mount and user namespace during path lookup.

Then check doesn't make sense at that point. The copy in the namespace of the parent - which may be the host's - is now locked and the host cannot change the flags of its own mounts anymore.

Here's the reproducer:

- host has debugfs mounted nosuid,nodev,noexec and shared
- tracefs automount below it is not yet active
- hand a child process a directory descriptor on that mount
- child process enters auser and mount namespace
- child process' namespace now holds a locked copy of the host's debugfs mount that receives propagation from it
- child process stats "tracing/." through the descriptor
- lookup runs on the host's mount so the automount lands below the host's mount
- propagation puts a copy below the child's copy
- both try to clear the flags on the mount they got, with a bind remount:

    host, on its own automount:            MS_REMOUNT|MS_BIND = EPERM
    child, on the copy in its namespace:   MS_REMOUNT|MS_BIND = 0

So the lock landed on the host's mount instead of the child's copy. Congrats. So we need to compare with the owner of the namespace the mount actually gets mounted on. For all regular cases that is the caller's mount namespace and so nothing changes.

Detached trees in anonymous mount namespaces by be handed over via SCM_RIGHTS or inherited in other ways on purpose so the attaching task's mount namespace is authoritative, not the creator of the detached tree.

---
 fs/namespace.c |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

@@ -2589,11 +2589,11 @@ enum mnt_tree_flags_t {
 static int attach_recursive_mnt(struct mount *source_mnt,
 				const struct pinned_mountpoint *dest)
 {
-	struct user_namespace *user_ns = current->nsproxy->mnt_ns->user_ns;
 	struct mount *dest_mnt = dest->parent;
 	struct mountpoint *dest_mp = dest->mp;
 	HLIST_HEAD(tree_list);
 	struct mnt_namespace *ns = dest_mnt->mnt_ns;
+	struct user_namespace *user_ns = ns->user_ns;
 	struct pinned_mountpoint root = {};
 	struct mountpoint *shorter = NULL;
 	struct mount *child, *p;

@@ -2603,6 +2603,22 @@ static int attach_recursive_mnt(struct m
 	bool moving = mnt_has_parent(source_mnt);
 
 	/*
+	 * A caller in an unprivileged mount namespaces may trigger an
+	 * automount and propagate locked mounts into privileged mount
+	 * namespaces. Take ownership from the target mount namespace.
+	 * It's equivalent for everything but the automount case.
+	 *
+	 * Detached trees in anonymous mount namespaces by be handed
+	 * over via SCM_RIGHTS or inherited in other ways on purpose
+	 * the attaching task's mount namespace is authoritative, not
+	 * the creator of the detached tree.
+	 */
+	if (is_anon_ns(ns))
+		user_ns = current->nsproxy->mnt_ns->user_ns;
+	else
+		user_ns = ns->user_ns;
+
+	/*
 	 * Preallocate a mountpoint in case the new mounts need to be
 	 * mounted beneath mounts on the same mountpoint.
 	 */

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

commit d74d66aa708ccf2bc0b1e265a83fed0f2308d5c4 upstream.

attach_recursive_mnt() transfers MNT_LOCKED from the top mount to the mount that is moved beneath it with MOVE_MOUNT_BENEATH. This allows the owner of a user namespace to replace its locked /proc or its root. The mount beneath takes on the job of covering the underlying mountpoint allowing the top mount to be unmounted.

Consider two mount namespaces:

(H) The host H has a shared mount P with a secret in P/d
(Z) Z is a user namespace made from M. Its copy of P receives propagation from the host's P and its copy of M's cover on P/d is locked Z's owner must not get to see P/d.

Now (H) mounts X on P/d. This propagates into (Z). The copy of X lands beneath (Z)'s locked cover. The locked property is transfered from (Z)'s cover to the copy of X propagated beneath it. The cover is now unlocked.

Now (H) unmounts X again. The copy of X in (Z) gets unmounted and the covering mount is left unlocked on top of P/d. (Z) can now unmount it:

    Z: umount2(P/d)             = EINVAL            /* the cover is locked */
    H: mount X on P/d, umount X                     /* both propagate into Z /*
    Z: umount2(P/d)             = 0                 /* the cover is now unlocked */

Z: read P/d/secret = "covered-by-root" /* secret revealed */

So only transfer the locked property to the mount beneath for mounts the caller has placed. A propagated copy that lands beneath a locked mount is locked as well so that the mount at the bottom of the stack carries a lock the way every check expects. The mount on top of it remains locked to ensure that it keeps covering even if the propagated mount is unmounted again.

---
 fs/namespace.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

@@ -2697,15 +2697,17 @@ static int attach_recursive_mnt(struct m
 			/*
 			 * If @q was locked it was meant to hide
 			 * whatever was under it. Let @child take over
-			 * that job and lock it, then we can unlock @q.
-			 * That'll allow another namespace to shed @q
-			 * and reveal @child. Clearly, that mounter
-			 * consented to this by not severing the mount
-			 * relationship. Otherwise, what's the point.
+			 * that job and lock it. If @child is the mount
+			 * the caller placed we can then unlock @q:
+			 * nothing another namespace does removes it
+			 * again. A propagated copy goes away when the
+			 * mounter of the original unmounts it, so @q
+			 * keeps its lock.
 			 */
 			if (IS_MNT_LOCKED(q)) {
 				child->mnt.mnt_flags |= MNT_LOCKED;
-				q->mnt.mnt_flags &= ~MNT_LOCKED;
+				if (child == source_mnt)
+					q->mnt.mnt_flags &= ~MNT_LOCKED;
 			}
 			mnt_change_mountpoint(r, mp, q);
 		}

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Norbert Szetei <[email protected]>

[ Upstream commit d84d6c1a7f1e2de9e601703a326a4c1b7b421943 ]

When an exchange covers both files from offset 0 to i_disk_size, xmi_can_exchange_reflink_flags() assumes every mapping moved and hands the reflink flag over to the other file.

That assumption is wrong. A file can still own shared blocks past i_disk_size, e.g. after EXCHANGE_RANGE_TO_EOF sets a smaller size without unmapping anything. The flag is cleared anyway, and the next write to such a block happens in place instead of CoW, corrupting the other file that shares it.

Commit a23eca88448e ("xfs: fix exchange-range reflink flag clearing issue with INO1_WRITTEN") fixed one instance of this. Rather than add another special case, stop moving the flags. xfs_exchmaps_ensure_reflink() already sets the flag on both inodes before the exchange, so both just keep it.

Nothing sets CLEAR_INO{1,2}_REFLINK anymore. Intents logged by older kernels may still carry them; recovery already masks them off, so they stay in the log format mask and are ignored.

XFS_IOC_SWAPEXT has the same problem: it moves the reflink flag from one inode to the other along with the extents. Just removing that is not enough here: the exchange path flags both inodes up front in xfs_exchmaps_ensure_reflink(), but swapext has no such step, so the inode receiving the shared extents would be left without the flag. Set it on both inodes instead, and make sure both have a CoW fork, since an inode with the reflink flag is expected to have one.

The flag now stays until FALLOC_FL_UNSHARE_RANGE, xfs_scrub, or a truncate to zero removes it.

---
 fs/xfs/libxfs/xfs_exchmaps.c   |   90 +----------------------------------------
 fs/xfs/libxfs/xfs_log_format.h |    4 -
 fs/xfs/xfs_bmap_util.c         |   15 ++----
 3 files changed, 10 insertions(+), 99 deletions(-)

@@ -49,7 +49,7 @@ struct xfs_exchmaps_adjacent {
 	.right2 = { .br_startblock = HOLESTARTBLOCK }, \
 }
 
-/* Information to reset reflink flag / CoW fork state after an exchange. */
+/* Information to reset CoW fork state after an exchange. */
 
 /*
  * If the reflink flag is set on either inode, make sure it has an incore CoW

@@ -127,9 +127,7 @@ xmi_has_more_exchange_work(const struct
 static inline bool
 xmi_has_postop_work(const struct xfs_exchmaps_intent *xmi)
 {
-	return xmi->xmi_flags & (XFS_EXCHMAPS_CLEAR_INO1_REFLINK |
-				 XFS_EXCHMAPS_CLEAR_INO2_REFLINK |
-				 __XFS_EXCHMAPS_INO2_SHORTFORM);
+	return xmi->xmi_flags & __XFS_EXCHMAPS_INO2_SHORTFORM;
 }
 
 /* Check all mappings to make sure we can actually exchange them. */

@@ -525,18 +523,6 @@ free:
 	return error;
 }
 
-/* Clear the reflink flag after an exchange. */
-static inline void
-xfs_exchmaps_clear_reflink(
-	struct xfs_trans	*tp,
-	struct xfs_inode	*ip)
-{
-	trace_xfs_reflink_unset_inode_flag(ip);
-
-	ip->i_diflags2 &= ~XFS_DIFLAG2_REFLINK;
-	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-}
-
 /* Finish whatever work might come after an exchange operation. */
 static int
 xfs_exchmaps_do_postop_work(

@@ -557,16 +543,6 @@ xfs_exchmaps_do_postop_work(
 			return error;
 	}
 
-	if (xmi->xmi_flags & XFS_EXCHMAPS_CLEAR_INO1_REFLINK) {
-		xfs_exchmaps_clear_reflink(tp, xmi->xmi_ip1);
-		xmi->xmi_flags &= ~XFS_EXCHMAPS_CLEAR_INO1_REFLINK;
-	}
-
-	if (xmi->xmi_flags & XFS_EXCHMAPS_CLEAR_INO2_REFLINK) {
-		xfs_exchmaps_clear_reflink(tp, xmi->xmi_ip2);
-		xmi->xmi_flags &= ~XFS_EXCHMAPS_CLEAR_INO2_REFLINK;
-	}
-
 	return 0;
 }
 

@@ -948,56 +924,13 @@ xfs_exchmaps_intent_destroy_cache(void)
 }
 
 /*
- * Decide if we will exchange the reflink flags between the two files after the
- * exchange.  The only time we want to do this is if we're exchanging all
- * mappings under EOF and the inode reflink flags have different states.
+ * Allocate and initialize a new incore intent item from a request.
  */
-static inline bool
-xmi_can_exchange_reflink_flags(
-	const struct xfs_exchmaps_req	*req,
-	unsigned int			reflink_state)
-{
-	struct xfs_mount		*mp = req->ip1->i_mount;
-
-	/*
-	 * The INO1_WRITTEN optimization can skip exchanging hole and
-	 * unwritten mappings, which means we cannot guarantee that all
-	 * shared extents actually moved to the other file.  Clearing the
-	 * reflink flag of an inode that still holds shared extents breaks
-	 * the CoW write path, so refuse to exchange the flags in that case.
-	 */
-	if (req->flags & XFS_EXCHMAPS_INO1_WRITTEN)
-		return false;
-
-	/*
-	 * The INO1_WRITTEN optimization can skip exchanging hole and
-	 * unwritten mappings, which means we cannot guarantee that all
-	 * shared extents actually moved to the other file.  Clearing the
-	 * reflink flag of an inode that still holds shared extents breaks
-	 * the CoW write path, so refuse to exchange the flags in that case.
-	 */
-	if (req->flags & XFS_EXCHMAPS_INO1_WRITTEN)
-		return false;
-
-	if (hweight32(reflink_state) != 1)
-		return false;
-	if (req->startoff1 != 0 || req->startoff2 != 0)
-		return false;
-	if (req->blockcount != XFS_B_TO_FSB(mp, req->ip1->i_disk_size))
-		return false;
-	if (req->blockcount != XFS_B_TO_FSB(mp, req->ip2->i_disk_size))
-		return false;
-	return true;
-}
-
-
-/* Allocate and initialize a new incore intent item from a request. */
 struct xfs_exchmaps_intent *
 xfs_exchmaps_init_intent(
 	const struct xfs_exchmaps_req	*req)
 {
 	struct xfs_exchmaps_intent	*xmi;
-	unsigned int			rs = 0;
 
 	xmi = kmem_cache_zalloc(xfs_exchmaps_intent_cache,
 			GFP_NOFS | __GFP_NOFAIL);

@@ -1021,23 +954,6 @@ xfs_exchmaps_init_intent(
 		xmi->xmi_isize2 = req->ip1->i_disk_size;
 	}
 
-	/* Record the state of each inode's reflink flag before the op. */
-	if (xfs_is_reflink_inode(req->ip1))
-		rs |= 1;
-	if (xfs_is_reflink_inode(req->ip2))
-		rs |= 2;
-
-	/*
-	 * Figure out if we're clearing the reflink flags (which effectively
-	 * exchanges them) after the operation.
-	 */
-	if (xmi_can_exchange_reflink_flags(req, rs)) {
-		if (rs & 1)
-			xmi->xmi_flags |= XFS_EXCHMAPS_CLEAR_INO1_REFLINK;
-		if (rs & 2)
-			xmi->xmi_flags |= XFS_EXCHMAPS_CLEAR_INO2_REFLINK;
-	}
-
 	if (S_ISDIR(VFS_I(xmi->xmi_ip2)->i_mode) ||
 	    S_ISLNK(VFS_I(xmi->xmi_ip2)->i_mode))
 		xmi->xmi_flags |= __XFS_EXCHMAPS_INO2_SHORTFORM;

@@ -927,10 +927,8 @@ struct xfs_xmi_log_format {
  */
 #define XFS_EXCHMAPS_INO1_WRITTEN	(1ULL << 2)
 
-/* Clear the reflink flag from inode1 after the operation. */
+/* No longer set, but older intents may carry them; keep them valid. */
 #define XFS_EXCHMAPS_CLEAR_INO1_REFLINK	(1ULL << 3)
-
-/* Clear the reflink flag from inode2 after the operation. */
 #define XFS_EXCHMAPS_CLEAR_INO2_REFLINK	(1ULL << 4)
 
 #define XFS_EXCHMAPS_LOGGED_FLAGS	(XFS_EXCHMAPS_ATTR_FORK | \

@@ -1514,7 +1514,6 @@ xfs_swap_extents(
 	struct xfs_bstat	*sbp = &sxp->sx_stat;
 	int			src_log_flags, target_log_flags;
 	int			error = 0;
-	uint64_t		f;
 	int			resblks = 0;
 	unsigned int		flags = 0;
 	struct timespec64	ctime, mtime;

@@ -1670,14 +1669,12 @@ xfs_swap_extents(
 	if (error)
 		goto out_trans_cancel;
 
-	/* Do we have to swap reflink flags? */
-	if ((ip->i_diflags2 & XFS_DIFLAG2_REFLINK) ^
-	    (tip->i_diflags2 & XFS_DIFLAG2_REFLINK)) {
-		f = ip->i_diflags2 & XFS_DIFLAG2_REFLINK;
-		ip->i_diflags2 &= ~XFS_DIFLAG2_REFLINK;
-		ip->i_diflags2 |= tip->i_diflags2 & XFS_DIFLAG2_REFLINK;
-		tip->i_diflags2 &= ~XFS_DIFLAG2_REFLINK;
-		tip->i_diflags2 |= f & XFS_DIFLAG2_REFLINK;
+	/* Either inode may now own shared blocks; flag both, with CoW forks. */
+	if ((ip->i_diflags2 | tip->i_diflags2) & XFS_DIFLAG2_REFLINK) {
+		ip->i_diflags2 |= XFS_DIFLAG2_REFLINK;
+		tip->i_diflags2 |= XFS_DIFLAG2_REFLINK;
+		xfs_ifork_init_cow(ip);
+		xfs_ifork_init_cow(tip);
 	}
 
 	/* Swap the cow forks. */

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: "Darrick J. Wong" <[email protected]>

[ Upstream commit 7dfb218c829c1acb626e0fe92c5fddb9a26a03e9 ]

Norbert Szetei posted a patch containing an incomplete description of a bug in XFS_IOC_EXCHANGE_RANGE's TO_EOF flag. The bug tricks exchange-range into modifying a file so that it has shared blocks starting beyond EOF, which is never allowed because files never have written data beyond EOF, and you can only share written data blocks. This is key to all the incorrect behavior that follows.

Eventually I got from Norbert a description of what's going wrong:

So yes, TO_EOF is screwing up the file size even if it's actually exchanging the data block correctly. The following is the intended behavior of XFS_EXCHANGE_RANGE_TO_EOF, as described by its manpage:

"Ignore the length parameter. All bytes in file1_fd from file1_offset
to EOF are moved to file2_fd, and file2's size is set to (file2_offset+(file1_length-file1_offset)). Meanwhile, all bytes in file2 from file2_offset to EOF are moved to file1 and file1's size is set to (file1_offset+(file2_length-file2_offset))."

In other words, if we only exchange a portion of the file data, then we should only exchange the portion of the file size that relates to the exchanged data. Unfortunately, the code swaps the ondisk file size and the incore file sizes, which is incorrect if the caller passes in a nonzero offset.

Having confused the file sizes, the PoC uses a second exchange-range call on what looks like a non-reflinked single-block file. Because the file size is set incorrectly, exchange-range thinks it's exchanging the full contents of two files and clears the reflink flag on the broken file. That enables an extending write of the broken file to rewrite the shared block that's just past EOF. This has become known colloquially as refluxfs.

Once the file sizes are set correctly, the second exchange-range no longer thinks that it's doing a full-contents swap, so it won't clear the reflink flag on either of its file arguments.

Norbert followed up to an earlier version of this patch with a second complaint about us not handling the i_disk_size state correctly:

That's wrong, and the new file size update logic depends on the ondisk i_disk_size matching the incore i_size, so flush the pagecache as needed to push it upwards.

---
 fs/xfs/libxfs/xfs_exchmaps.c |   13 +++++++++-
 fs/xfs/xfs_exchrange.c       |   51 +++++++++++++++++++++++++++++++------------
 2 files changed, 48 insertions(+), 16 deletions(-)

@@ -931,6 +931,7 @@ xfs_exchmaps_init_intent(
 	const struct xfs_exchmaps_req	*req)
 {
 	struct xfs_exchmaps_intent	*xmi;
+	struct xfs_mount		*mp = req->ip1->i_mount;
 
 	xmi = kmem_cache_zalloc(xfs_exchmaps_intent_cache,
 			GFP_NOFS | __GFP_NOFAIL);

@@ -948,10 +949,18 @@ xfs_exchmaps_init_intent(
 		return xmi;
 	}
 
+	/*
+	 * If the caller wanted, set each file's size to that file's exchange
+	 * offset + length exchanged from the other file because the ranges in
+	 * each file might be different lengths.
+	 */
 	if (req->flags & XFS_EXCHMAPS_SET_SIZES) {
+		loff_t	off1 = XFS_FSB_TO_B(mp, xmi->xmi_startoff1);
+		loff_t	off2 = XFS_FSB_TO_B(mp, xmi->xmi_startoff2);
+
 		xmi->xmi_flags |= XFS_EXCHMAPS_SET_SIZES;
-		xmi->xmi_isize1 = req->ip2->i_disk_size;
-		xmi->xmi_isize2 = req->ip1->i_disk_size;
+		xmi->xmi_isize1 = off1 + (req->ip2->i_disk_size - off2);
+		xmi->xmi_isize2 = off2 + (req->ip1->i_disk_size - off1);
 	}
 
 	if (S_ISDIR(VFS_I(xmi->xmi_ip2)->i_mode) ||

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Luca Weiss <[email protected]>

[ Upstream commit 5ffd02d16f4e2f0566b8f31d5610538987e1944c ]

The critical_cbrcrs array will get qcom_branch_set_clk_en() called on each entry which will set BIT(0) to enable the clock. This is however wrong for these two PCIE_RSCC clocks which need BIT(20) and BIT(21) set respectively to enable them.

---
 drivers/clk/qcom/gcc-eliza.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 3e26c7a1e5b54..fe18942a59ac4 100644
@@ -3010,8 +3010,6 @@ static const u32 gcc_eliza_critical_cbcrs[] = {
 	0x26034, /* GCC_CAMERA_XO_CLK */
 	0x27004, /* GCC_DISP_AHB_CLK */
 	0x71004, /* GCC_GPU_CFG_AHB_CLK */
-	0x52010, /* GCC_PCIE_RSCC_CFG_AHB_CLK */
-	0x52010, /* GCC_PCIE_RSCC_XO_CLK */
 	0x32004, /* GCC_VIDEO_AHB_CLK */
 	0x32038, /* GCC_VIDEO_XO_CLK */
 };

@@ -3045,6 +3043,10 @@ static const struct regmap_config gcc_eliza_regmap_config = {
 
 static void clk_eliza_regs_configure(struct device *dev, struct regmap *regmap)
 {
+	/* Keep clocks always enabled */
+	regmap_update_bits(regmap, 0x52010, BIT(20), BIT(20)); /* GCC_PCIE_RSCC_CFG_AHB_CLK */
+	regmap_update_bits(regmap, 0x52010, BIT(21), BIT(21)); /* GCC_PCIE_RSCC_XO_CLK */
+
 	/* FORCE_MEM_CORE_ON for ufs phy ice core and gcc ufs phy axi clocks  */
 	qcom_branch_set_force_mem_core(regmap, gcc_ufs_phy_ice_core_clk, true);
 	qcom_branch_set_force_mem_core(regmap, gcc_ufs_phy_axi_clk, true);

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Luca Weiss <[email protected]>

[ Upstream commit 0bfb542fc3368371cefb4cf45ef21c3ba2d37c3f ]

The critical_cbrcrs array will get qcom_branch_set_clk_en() called on each entry which will set BIT(0) to enable the clock. This is however wrong for these two PCIE_RSCC clocks which need BIT(20) and BIT(21) set respectively to enable them.

---
 drivers/clk/qcom/gcc-kaanapali.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 842c1a70c6916..5bcbcd1786577 100644
@@ -3464,8 +3464,6 @@ static const u32 gcc_kaanapali_critical_cbcrs[] = {
 	0x9f004, /* GCC_EVA_AHB_CLK */
 	0x9f024, /* GCC_EVA_XO_CLK */
 	0x71004, /* GCC_GPU_CFG_AHB_CLK */
-	0x52010, /* GCC_PCIE_RSCC_CFG_AHB_CLK */
-	0x52010, /* GCC_PCIE_RSCC_XO_CLK */
 	0x32004, /* GCC_VIDEO_AHB_CLK */
 	0x32040, /* GCC_VIDEO_XO_CLK */
 };

@@ -3480,6 +3478,10 @@ static const struct regmap_config gcc_kaanapali_regmap_config = {
 
 static void clk_kaanapali_regs_configure(struct device *dev, struct regmap *regmap)
 {
+	/* Keep clocks always enabled */
+	regmap_update_bits(regmap, 0x52010, BIT(20), BIT(20)); /* GCC_PCIE_RSCC_CFG_AHB_CLK */
+	regmap_update_bits(regmap, 0x52010, BIT(21), BIT(21)); /* GCC_PCIE_RSCC_XO_CLK */
+
 	/* FORCE_MEM_CORE_ON for ufs phy ice core clocks */
 	qcom_branch_set_force_mem_core(regmap, gcc_ufs_phy_ice_core_clk, true);
 }

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Taniya Das <[email protected]>

[ Upstream commit 8f3f88309fa4e975ac27368ee945c2c202251a7c ]

The GPU CX GDSC on Glymur is a votable power domain and hence should not be polled for power-off status during GDSC disable. Also, its power-on status should be read from the GDS hardware control register.

Accommodate this by removing POLL_CFG_GDSCR and adding the VOTABLE flag for the GPU CX GDSC, similar to other platforms (e.g. SM8750 and Milos).

---
 drivers/clk/qcom/gpucc-glymur.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 001b2454786a0..cac51de20b295 100644
@@ -510,7 +510,7 @@ static struct gdsc gpu_cc_cx_gdsc = {
 		.name = "gpu_cc_cx_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE,
+	.flags = RETAIN_FF_ENABLE | VOTABLE,
 };
 
 static struct clk_regmap *gpu_cc_glymur_clocks[] = {

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Taniya Das <[email protected]>

[ Upstream commit 55981579b27b6541aeeab81c768f248dba0491ac ]

The GPU CX GDSC on Kaanapali is a votable power domain and hence should not be polled for power-off status during GDSC disable. Also, its power-on status should be read from the GDS hardware control register.

Accommodate this by removing POLL_CFG_GDSCR and adding the VOTABLE flag for the GPU CX GDSC, similar to other platforms (e.g. SM8750 and Milos).

---
 drivers/clk/qcom/gpucc-kaanapali.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ae5563e516f67..f75f2caa2205c 100644
@@ -380,7 +380,7 @@ static struct gdsc gpu_cc_cx_gdsc = {
 		.name = "gpu_cc_cx_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE,
+	.flags = RETAIN_FF_ENABLE | VOTABLE,
 };
 
 static struct clk_regmap *gpu_cc_kaanapali_clocks[] = {

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Vincent Jardin <[email protected]>

[ Upstream commit 8188a59dd6782786556b18f515090b79cb701c45 ]

For gpio1 value set for pinctrl is outside the mask and so will do nothing.

Add the missing offset for value field to be the same as the mask as expected.

---
 arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 1d73abffa6b72..b9f079c49e193 100644
@@ -1803,7 +1803,7 @@ fspi_data74_pins: xspi1-data74-pins {
 			};
 
 			gpio1_31_28_pins: xspi1-data74-gpio-pins {
-				pinctrl-single,bits = <0x0 0x1 (0x7 << 15)>;
+				pinctrl-single,bits = <0x0 (0x1 << 15) (0x7 << 15)>;
 			};
 
 			fspi_data30_pins: xspi1-data30-pins {

@@ -1811,7 +1811,7 @@ fspi_data30_pins: xspi1-data30-pins {
 			};
 
 			gpio1_27_24_pins: xspi1-data30-gpio-pins {
-				pinctrl-single,bits = <0x0 0x1 (0x7 << 18)>;
+				pinctrl-single,bits = <0x0 (0x1 << 18) (0x7 << 18)>;
 			};
 
 			fspi_dqs_sck_cs10_pins: xspi1-base-pins {

@@ -1819,7 +1819,7 @@ fspi_dqs_sck_cs10_pins: xspi1-base-pins {
 			};
 
 			gpio1_23_20_pins: xspi1-base-gpio-pins {
-				pinctrl-single,bits = <0x0 0x1 (0x7 << 21)>;
+				pinctrl-single,bits = <0x0 (0x1 << 21) (0x7 << 21)>;
 			};
 
 			esdhc0_cmd_data30_clk_vsel_pins: sdhc1-base-sdhc-vsel-pins {

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Vincent Jardin <[email protected]>

[ Upstream commit 6affdda171ca8f2e082e8c4fba56233a2f8c9910 ]

IIC1_PMUX is a single bit field at RCWSR14 bit 10, so the mask for the two iic1 pinmux nodes was narrowed from 0x7 to 0x1 to avoid writing the two reserved bits next to it.

With pinctrl-single,bit-per-mux the driver does not take the supplied mask as the bits to modify. It derives the field from the node's pinctrl-single,function-mask, which is 0x7 here, and then requires the supplied mask to match it exactly:

	bit_pos = __ffs(mask);			/* 10 */
	mask_pos = ((pcs->fmask) << bit_pos);	/* 0x7 << 10 = 0x1c00 */
	submask = mask & mask_pos;		/* 0x400 */
	...
	if (submask != mask_pos) {
		dev_warn(pcs->dev,
			 "Invalid submask 0x%x for %pOFn at 0x%x\n",
			 submask, np, offset);
		continue;
	}

A one bit mask gives submask 0x400 against mask_pos 0x1c00, so both nodes are rejected and the pins are never muxed with this message:

pinctrl-single 70010012c.pinmux: Invalid submask 0x400 for iic1-i2c-pins pinctrl-single 70010012c.pinmux: Invalid submask 0x400 for iic1-gpio-pins

The values are not changed:
- 0x0 selects IIC1
- 0x1 << 10 selects GPIO_1[3:2].

---
 arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index b9f079c49e193..8f33dd26577fa 100644
@@ -1868,12 +1868,19 @@ i2c6_i2c7_pins: iic8-iic7-i2c-pins {
 			};
 
 			/* RCWSR14 */
+			/*
+			 * IIC1_PMUX is a single bit at RCWSR14[10], but bit-per-mux
+			 * uses the mask for the field to clear before applying the
+			 * value.
+			 * So bits 11 and 12, which are reserved, are written as 0
+			 * on every mux switch, it is harmless.
+			 */
 			i2c0_pins: iic1-i2c-pins {
-				pinctrl-single,bits = <0x8 0x0 (0x1 << 10)>;
+				pinctrl-single,bits = <0x8 0x0 (0x7 << 10)>;
 			};
 
 			gpio0_3_2_pins: iic1-gpio-pins {
-				pinctrl-single,bits = <0x8 (0x1 << 10) (0x1 << 10)>;
+				pinctrl-single,bits = <0x8 (0x1 << 10) (0x7 << 10)>;
 			};
 		};
 

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Vincent Jardin <[email protected]>

[ Upstream commit f14f4dc8ea3810f8ed4f0f3ea13f8bba3e2b0f04 ]

The iic5-spi3-pins node has both of its first two cells wrong.

The offset selects the register within the node's reg range, where:
0x0 is RCWSR12,
0x4 is RCWSR13,
0x8 is RCWSR14.

An offset of 0x3 is not one of them and is not even aligned for the declared 32 bit register width. It is the only node in this pinmux that does not use 0x0, 0x4 neither 0x8.

The value is wrong too:
IIC5_PMUX is RCWSR12[11:9] and it encodes 0b000 for IIC5, 0b001 for GPIO_1[25:24],
0b010 for SDHC1_CLK_SYNC_OUT/IN,
0b011 for {IIC5_SCL,IIC5_SDA} = {SPI3_SOUT,SPI3_SIN}.

This node selects the last of those, so it needs 0x3, not the 0x2, which is the value of the neighbouring sdhc1 clock sync node it was copied from.

---
 arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8f33dd26577fa..6ead557a8cf19 100644
@@ -1783,7 +1783,7 @@ esdhc0_clksync_pins: iic5-sdhc-clk-pins {
 			};
 
 			dspi2_miso_mosi_pins: iic5-spi3-pins {
-				pinctrl-single,bits = <0x3 (0x2 << 9) (0x7 << 9)>;
+				pinctrl-single,bits = <0x0 (0x3 << 9) (0x7 << 9)>;
 			};
 
 			i2c5_pins: iic6-i2c-pins {

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Peng Fan <[email protected]>

[ Upstream commit 057e5e07420c753f248f9ce148040ab6dcf8359f ]

MMIO resources mapped via dma_map_resource() carry DMA_ATTR_MMIO. These never need swiotlb bounce buffering, but the current code enters the swiotlb path when the physical address is not page-aligned, then rejects the mapping because MMIO cannot be bounced.

Move the DMA_ATTR_MMIO check before the swiotlb block so the entire bounce path is skipped for MMIO, falling through directly to __iommu_dma_map() which creates the IOVA mapping.

---
 drivers/iommu/dma-iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index ae814d8c99c04..5f901b4baf216 100644
@@ -1228,9 +1228,9 @@ dma_addr_t iommu_dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
 	 * If both the physical buffer start address and size are page aligned,
 	 * we don't need to use a bounce page.
 	 */
-	if (dev_use_swiotlb(dev, size, dir) &&
+	if (!(attrs & DMA_ATTR_MMIO) && dev_use_swiotlb(dev, size, dir) &&
 	    iova_unaligned(iovad, phys, size)) {
-		if (attrs & (DMA_ATTR_MMIO | DMA_ATTR_REQUIRE_COHERENT))
+		if (attrs & DMA_ATTR_REQUIRE_COHERENT)
 			return DMA_MAPPING_ERROR;
 
 		phys = iommu_dma_map_swiotlb(dev, phys, size, dir, attrs);

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

[ Upstream commit a2b42babc912e4fb428cbb42adb4f6e1e067113d ]

do_set_group() only accepts mounts as targets that are neither shared nor a slave. That encompasses undindable mounts.

If the source mount is a slave mount the target mount will end up with source's master as its master. It keeps T_UNBINDABLE. That means we get a mount that is both unbindable and a slave:

  mount --bind /tmp/src /tmp/src
  mount --make-shared /tmp/src
  mount --bind /tmp/src /tmp/b
  mount --make-slave /tmp/b
  mount --bind /tmp/src /tmp/c
  mount --make-unbindable /tmp/c
  move_mount(b, "", c, "", MOVE_MOUNT_SET_GROUP)
  grep /tmp/c /proc/self/mountinfo
  ... /tmp/c ... master:646 unbindable ...

This property cannot be produced any other way. change_mnt_propagation() drops the master when it makes a mount unbindable (iow, it becomes private) and doesn't touch an unbindable mount when it is supposed to turned into a slave mount.

End-result is that the unbindable-slave mount receives everything propagated from its master's peer group while it can't be bind mounted itself. Not sure what that's supposed to do.

On the other side: if the source mount is shared, the target mount drops T_UNBINDABLE because because set_mnt_shared() clears T_SHARED_MASK.

So teach do_set_group() to refuse an unbindable target mount the same way a shared or slave target mount is refused.

The main user of MOVE_MOUNT_SET_GROUP is CRIU which restores sharing first and applies MS_UNBINDABLE afterwards. It never hits this.

---
 fs/namespace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 472cca0b730d9..010bc659188d4 100644
@@ -3530,7 +3530,7 @@ static int do_set_group(const struct path *from_path, const struct path *to_path
 		return -EINVAL;
 
 	/* Setting sharing groups is only allowed on private mounts */
-	if (IS_MNT_SHARED(to) || IS_MNT_SLAVE(to))
+	if (IS_MNT_SHARED(to) || IS_MNT_SLAVE(to) || IS_MNT_UNBINDABLE(to))
 		return -EINVAL;
 
 	/* From should not be private */

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Christian Brauner <[email protected]>

[ Upstream commit 7eb84d54fac5002bd975d1de8fd61d57e89172ab ]

propagate_mount_busy() uses mnt_get_count() via do_refcount_check() to figure out whether a synchronous umount may proceed or not by summing the per-CPU mnt_count counter under mount_lock.

The fastpath for mntget() and mntput() doesn't require that lock. A task that already holds a reference and takes another one on a cpu the loop has summed can get migrated and drops that reference again on another cpu. The loop might not have reached that cpu yet. That ends up undercounting. The original reference of the task is missed and umount() ends up succeeding with the file still open. The mount is destroyed later when the task mntput()s so it degrades to a lazy umount.

That's basically the umount(2) variant of commit 9ea0a46ca2c3 ("fix mntput/mntput race") closed for the final mntput(). The same cpu-migration caused a UAF.

Luckily this isn't a UAF. It's just a hidden reference pinning the mount but it's wrong nonetheless.

Like the other race it's a narrow window but I can reproduce it reliably with some patching. The debug patch below prints the sum the loop computed next to an immediate re-sum of the same counters. It reports who drops the last reference to that mount later:

#  --- a/fs/namespace.c
#  +++ b/fs/namespace.c
#  @@ -34,6 +34,8 @@
#   #include <linux/mnt_idmapping.h>
#   #include <linux/pidfs.h>
#   #include <linux/nstree.h>

# +#include <linux/debugfs.h>
# +#include <linux/delay.h>
#
# #include "pnode.h"
# #include "internal.h"
# @@ -263,17 +265,48 @@
# #endif
# }
#

#  +static u32 mnt_get_count_delay_ms;
#  +static int mnt_get_count_debug_id;
#  +
#  +static int __init mnt_get_count_debug_init(void)
#  +{
#  +	debugfs_create_u32("mnt_get_count_delay_ms", 0600, NULL,
#  +			   &mnt_get_count_delay_ms);
#  +	return 0;
#  +}
#  +late_initcall(mnt_get_count_debug_init);
#  +
#   /*
#    * vfsmount lock must be held for write
#    */
#   int mnt_get_count(struct mount *mnt)
#   {
#   #ifdef CONFIG_SMP
#  -	int count = 0;
#  +	static int seen[NR_CPUS];
#  +	bool slow = mnt_get_count_delay_ms &&
#  +		    !(mnt->mnt.mnt_flags & MNT_UMOUNT);
#  +	int count = 0, again = 0;
#   	int cpu;
#
#   	for_each_possible_cpu(cpu) {
#  -		count += per_cpu_ptr(mnt->mnt_pcp, cpu)->mnt_count;
#  +		seen[cpu] = per_cpu_ptr(mnt->mnt_pcp, cpu)->mnt_count;
#  +		count += seen[cpu];
#  +		if (slow)
#  +			mdelay(mnt_get_count_delay_ms);
#  +	}
#  +	if (slow) {
#  +		for_each_possible_cpu(cpu) {
#  +			int now;
#  +
#  +			now = per_cpu_ptr(mnt->mnt_pcp, cpu)->mnt_count;
#  +			again += now;
#  +			pr_err("mount %d: cpu%d read %d, now %d\n",
#  +			       mnt->mnt_id, cpu, seen[cpu], now);
#  +		}
#  +		mnt_get_count_debug_id = mnt->mnt_id;
#  +		pr_err("mount %d: sum %d, re-sum %d, %ps\n",
#  +		       mnt->mnt_id, count, again,
#  +		       __builtin_return_address(0));

# }
#
# return count;
# @@ -1350,6 +1383,9 @@

#   	smp_mb();
#   	mnt_add_count(mnt, -1);
#   	count = mnt_get_count(mnt);
#  +	if (count == 0 && mnt->mnt_id == mnt_get_count_debug_id)
#  +		pr_err("mount %d: last put by %s (%d)\n", mnt->mnt_id,
#  +		       current->comm, task_pid_nr(current));
#   	if (count != 0) {
#   		WARN_ON(count < 0);
#   		rcu_read_unlock();

A task holds open a directory fd on a tmpfs. A synchronous rumount() of the tmpfs sums up. Concurrently a task calls fchdir() on that fd on CPU 0 and fchdir()s back to / on CPU 3. This will cause a path_get() and a path_put() with a migration in between without mount_lock held.

Adding a delay per CPU makes the count loop miss the get in a cpu it already visited but count the put in one it hasn't.

The reproducer, run as root with debugfs mounted:

  /*
   * gcc -static -o umount-torn umount-torn.c
   */
  #define _GNU_SOURCE
  #include <errno.h>
  #include <fcntl.h>
  #include <sched.h>
  #include <stdio.h>
  #include <string.h>
  #include <unistd.h>
  #include <sys/mount.h>
  #include <sys/stat.h>
  #include <sys/wait.h>

  #define KNOB "/sys/kernel/debug/mnt_get_count_delay_ms"
  #define MNT "/mnt/x"

static int kmsg;

  static void pin(int cpu)
  {
  	cpu_set_t s;

  	CPU_ZERO(&s);
  	CPU_SET(cpu, &s);
  	sched_setaffinity(0, sizeof(s), &s);
  }

  static void knob(const char *v)
  {
  	int fd = open(KNOB, O_WRONLY);

  	write(fd, v, strlen(v));
  	close(fd);
  }

  int main(void)
  {
  	int last = sysconf(_SC_NPROCESSORS_ONLN) - 1;
  	int p[2], fd, ret;
  	pid_t pid;
  	char c;

  	kmsg = open("/dev/kmsg", O_WRONLY);
  	mkdir(MNT, 0755);
  	mount("none", MNT, "tmpfs", 0, NULL);
  	fd = open(MNT "/f", O_WRONLY | O_CREAT, 0644);
  	write(fd, "still here\n", 11);
  	close(fd);

  	pipe(p);
  	pid = fork();
  	if (pid == 0) {
  		int dirfd = open(MNT, O_RDONLY | O_DIRECTORY);
  		int rootfd = open("/", O_RDONLY | O_DIRECTORY);
  		struct stat st;
  		char buf[16];

  		/* one get/put pair while the umount is summing */
  		usleep(150000);
  		pin(0);
  		fchdir(dirfd);		/* mntget() */
  		dprintf(kmsg, "holder %d: get on cpu %d\n", getpid(),
  			sched_getcpu());
  		pin(last);
  		fchdir(rootfd);		/* mntput() */
  		dprintf(kmsg, "holder %d: put on cpu %d\n", getpid(),
  			sched_getcpu());

  		/* the umount has returned, is the fd still good? */
  		read(p[0], &c, 1);
  		fd = openat(dirfd, "f", O_RDONLY);
  		ret = read(fd, buf, sizeof(buf));
  		dprintf(kmsg, "holder %d: fstat %d openat %d read %d\n",
  			getpid(), fstat(dirfd, &st), fd, ret);
  		_exit(0);
  	}

  	pin(1);
  	knob("100");
  	ret = umount2(MNT, 0);
  	knob("0");
  	if (ret)
  		dprintf(kmsg, "umount: %s\n", strerror(errno));
  	else
  		dprintf(kmsg, "umount: 0\n");
  	write(p[1], "x", 1);
  	waitpid(pid, NULL, 0);
  	return 0;
  }

The log shows the busy check summing 2 and when read right after it reads 3. The loop read CPU 0 before the get landed there and CPU 3 after the put did. The umount succeeds while there's still someone with an fd. The mount is then dropped when the fd goes:

  [    2.511944] holder 90: get on cpu 0
  [    2.512023] holder 90: put on cpu 3
  [    2.671388] mount 28: cpu0 read 1, now 2
  [    2.671398] mount 28: cpu1 read 2, now 2
  [    2.671401] mount 28: cpu2 read 0, now 0
  [    2.671404] mount 28: cpu3 read -1, now -1
  [    2.671407] mount 28: sum 2, re-sum 3, propagate_mount_busy
  [    2.671773] umount: 0
  [    2.672963] holder 90: fstat 0 openat 8 read 11
  [    2.673663] mount 28: last put by umount-torn (90)

Easy fix would be to order mntget() against the loop that sums up. But that's a full barrier and a flag test in mntget() afaict. No bueno... That would cost every path_get().

So learn from SRCU. Keep gets and puts in separate per-cpu counters and make mnt_get_count() sum all puts first and all gets second with a full barrier in between.

A put is always after its own get (hopefully...) independent of the cpu the task is on. So a put the first pass counted must have its get visible to the second pass. Ergo, we can't undercount.

This is the two pass sum srcu_readers_active_idx_check() does for srcu_locks and srcu_unlocks. mntget() keeps a single this_cpu_inc(). mntput() gains an smp_wmb() before its increment, a compiler barrier on x86 and a store fence on the weakly ordered architectures, since the two counters are two words and a put stored after a get could otherwise become visible before it. The counters are unsigned so the difference stays right after they wrap.

The count can still come out high when a get is counted whose put the first pass did not see. We don't care. A task can only take a new reference if:

(1) it holds one already
(2) through __legitimize_mnt() which the seqcount orders against the write side

So a get in this window means someone has a reference and -EBUSY is correct. The final mntput() is unchanged. Once ->mnt_ns is NULL every put takes the slow path under mount_lock and so no put is in flight while it counts. Here's an attempted proof for that crap:

(i) A put never becomes visible before its get. Within one task the smp_wmb() in mntput_no_expire() orders every earlier store before the increment of the puts.

Any task that migrated in between had its stores flushed by the scheduler. When that reference is handed to another task that's trivially true.

(ii) The double count in mnt_get_count() is ordered via smp_mb(). That
keeps every read of the puts counter before every read of the gets counter.

(iii) We never undercount by (ii).

(iv) There are no spurious counts/differences. A get is counted only if
it happened and a put is left out only if it hasn't happened when the counter was read. With (iii) this means it's equivalent to the old count whenever nothing is taken or dropped during the loop.

(v) do_umount() holds a reference and mount_lock hence. New references come from: (v.a) __legitimize_mnt() managed by the seqcount. Either it's smp_mb() made the get visible to the loop or it takes the lock and waits for the loop. By (iii) a mount with a additional references is refused. By (iv) refusing the umount just means that any additional reference existed, same as the old counter.

(vi) mntput_no_expire_slowpath() runs with ->mnt_ns already NULL. Hence,
every other put must take the slowpath as well and hangs on mount_lock.

The grace period in namespace_unlock() will make every fast path do a put that still saw ->mnt_ns.

No put is in flight during the loop.

A get that is in flight comes from a task holding another reference. That must be included by the count or from __legitimize_mnt() as in (v).

Ergo, zero means the last reference and non-zero means a reference is live.

(vii) may_umount() uses propagate_mount_busy() as well and
may_umount_tree() sums mnt_get_count() itself.

With this patch the race is dead:

  [    3.066786] mount 28: sum 3, re-sum 3, propagate_mount_busy
  [    3.105867] umount: Device or resource busy
  [    3.106047] holder 90: fstat 0 openat 8 read 11

---
 fs/mount.h     |  3 ++-
 fs/namespace.c | 48 ++++++++++++++++++++++++++++++------------------
 2 files changed, 32 insertions(+), 19 deletions(-)

index 03b41e68776fb..2223fb141499f 100644
@@ -33,7 +33,8 @@ struct mnt_namespace {
 } __randomize_layout;
 
 struct mnt_pcp {
-	int mnt_count;
+	unsigned int mnt_gets;
+	unsigned int mnt_puts;
 	int mnt_writers;
 };
 

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Naomi Chu <[email protected]>

[ Upstream commit 02a3178319639f650428164d4d003de4dc03e326 ]

Clock gating becomes possible as soon as ufshcd_init_clk_gating() has run, and from that point on the probe keeps accessing host registers without ever taking a clock reference. This has been safe only because of the state check in __ufshcd_release(): gate_work is not queued unless hba->ufshcd_state is UFSHCD_STATE_OPERATIONAL, and the promotion to that state used to happen after the last register access of the probe, at the end of ufshcd_probe_hba().

That is fragile: it only works while the promotion happens after the register accesses. Commit a390e6677f41 ("scsi: ufs: core: Expand the ufshcd_device_init(hba, true) call") changed that ordering by moving the promotion into ufshcd_init(), which schedules ufshcd_async_scan() afterwards. ufshcd_probe_hba() therefore now runs with the state already promoted, and it accesses host registers without holding a clock reference:

- On hosts with UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH it calls ufshcd_hba_stop() and ufshcd_hba_enable() before ufshcd_device_init() sets the state back to UFSHCD_STATE_RESET, and both read REG_CONTROLLER_ENABLE, so gated clocks stall there instead of just losing a write.

- It ends with an ufshcd_configure_auto_hibern8() write, which its other callers do take a clock reference for.

Take a clock reference as soon as clock gating has been initialised and keep it until the probe is over. It then does not matter who drops a clock reference while the probe is running, and the register accesses of the probe no longer depend on hba->ufshcd_state. The reference is dropped by ufshcd_async_scan() once the scan has finished, or by the new out_release label if the probe fails after it was taken.

---
 drivers/ufs/core/ufshcd.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

index fb5f119b19772..0a9366f21483f 100644
@@ -9484,6 +9484,7 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	ret = ufshcd_add_lus(hba);
 
 out:
+	ufshcd_release(hba);
 	pm_runtime_put_sync(hba->dev);
 
 	if (ret)

@@ -11206,6 +11207,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	ufshcd_init_clk_gating(hba);
 
+	/* Released by ufshcd_async_scan(), or by out_release on failure. */
+	ufshcd_hold(hba);
+
 	ufshcd_init_clk_scaling(hba);
 
 	/*

@@ -11226,7 +11230,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	err = devm_request_irq(dev, irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba);
 	if (err) {
 		dev_err(hba->dev, "request irq failed\n");
-		goto out_disable;
+		goto out_release;
 	} else {
 		hba->is_irq_enabled = true;
 	}

@@ -11242,7 +11246,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 		dev_err(hba->dev, "Host controller enable failed\n");
 		ufshcd_print_evt_hist(hba);
 		ufshcd_print_host_state(hba);
-		goto out_disable;
+		goto out_release;
 	}
 
 	INIT_DELAYED_WORK(&hba->rpm_dev_flush_recheck_work, ufshcd_rpm_dev_flush_recheck_work);

@@ -11256,7 +11260,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	err = ufshcd_add_scsi_host(hba);
 	if (err)
-		goto out_disable;
+		goto out_release;
 
 	/* Hold auto suspend until async scan completes */
 	pm_runtime_get_sync(dev);

@@ -11276,7 +11280,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	err = ufshcd_link_startup(hba);
 	if (err)
-		goto out_disable;
+		goto out_release;
 
 	if (hba->mcq_enabled)
 		ufshcd_config_mcq(hba);

@@ -11293,23 +11297,23 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	/* Verify device initialization by sending NOP OUT UPIU */
 	err = ufshcd_verify_dev_init(hba);
 	if (err)
-		goto out_disable;
+		goto out_release;
 
 	/* Initiate UFS initialization, and waiting until completion */
 	err = ufshcd_complete_dev_init(hba);
 	if (err)
-		goto out_disable;
+		goto out_release;
 
 	err = ufshcd_device_params_init(hba);
 	if (err)
-		goto out_disable;
+		goto out_release;
 
 	err = ufshcd_post_device_init(hba);
 
 initialized:
 	ufshcd_process_probe_result(hba, probe_start, err);
 	if (err)
-		goto out_disable;
+		goto out_release;
 
 	ufs_sysfs_add_nodes(hba->dev);
 	hba->dme_qos_sysfs_handle = sysfs_get_dirent(hba->dev->kobj.sd,

@@ -11320,6 +11324,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	ufshcd_pm_qos_init(hba);
 	return 0;
 
+out_release:
+	ufshcd_release(hba);
 out_disable:
 	hba->is_irq_enabled = false;
 	ufshcd_hba_exit(hba);

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Ravindra <[email protected]>

[ Upstream commit baa53af641c9e3644e717b8dfd1b95a55a0c111b ]

btintel_pcie_prepare_tx() uses tfd_index to access the TFD and data buffer arrays before the index is advanced modulo txq->count. An index equal to txq->count is one past the end of both arrays, so reject it in the bounds check.

---
 drivers/bluetooth/btintel_pcie.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ed2a6286892c5..34899a5507339 100644
@@ -401,7 +401,7 @@ static int btintel_pcie_send_sync(struct btintel_pcie_data *data,
 
 	tfd_index = data->ia.tr_hia[BTINTEL_PCIE_TXQ_NUM];
 
-	if (tfd_index > txq->count)
+	if (tfd_index >= txq->count)
 		return -ERANGE;
 
 	if (skb->len > BTINTEL_PCIE_BUFFER_SIZE - BTINTEL_PCIE_HCI_TYPE_LEN) {

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Sai Teja Aluvala <[email protected]>

[ Upstream commit 1465494e8d07426fa41c0021530cccb2a1ac38fa ]

Reset shared Bluetooth hardware after mapping the BAR and before setting up MSI-X interrupts, so stale device state is cleared before IRQ handlers and interrupt masks are configured.

Propagate reset errors so probe aborts rather than continuing when the reset fails.

---
 drivers/bluetooth/btintel_pcie.c | 7 +++++++
 1 file changed, 7 insertions(+)

index 34899a5507339..ed4ba2e469ac7 100644
@@ -1819,6 +1819,13 @@ static int btintel_pcie_config_pcie(struct pci_dev *pdev,
 	if (IS_ERR(data->base_addr))
 		return PTR_ERR(data->base_addr);
 
+	/* Do shared hardware reset to ensure a clean start before
+	 * configuring interrupts.
+	 */
+	err = btintel_pcie_reset_bt(data);
+	if (err)
+		return err;
+
 	err = btintel_pcie_setup_irq(data);
 	if (err)
 		return err;

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Cristian Ciocaltea <[email protected]>

[ Upstream commit b62e3db8cdc1680e1c3390591e872f485148541a ]

When re-adding a bridge that was previously removed, drm_bridge_add() drops it from bridge_lingering_list without holding bridge_lock.

Both bridge_list and bridge_lingering_list are protected by bridge_lock, as they are concurrently modified by drm_bridge_remove() and __drm_bridge_free(), and walked by the debugfs 'bridges' file. Running the list_empty() test and the list_del_init() outside of the lock may therefore corrupt either list.

Perform both under bridge_lock.

---
 drivers/gpu/drm/drm_bridge.c | 2 ++
 1 file changed, 2 insertions(+)

index 687b36eea0c70..3618cb12483e2 100644
@@ -414,8 +414,10 @@ void drm_bridge_add(struct drm_bridge *bridge)
 	 * in bridge_lingering_list. Remove it or bridge_lingering_list will be
 	 * corrupted when adding this bridge to bridge_list below.
 	 */
+	mutex_lock(&bridge_lock);
 	if (!list_empty(&bridge->list))
 		list_del_init(&bridge->list);
+	mutex_unlock(&bridge_lock);
 
 	mutex_init(&bridge->hpd_mutex);
 

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Cristian Ciocaltea <[email protected]>

[ Upstream commit 57889076afd1be890150c32ffde3b691fe5707c8 ]

drm_bridge_attach() inspects bridge->list to warn about bridges being attached without a previous drm_bridge_add() call, but it does so without holding bridge_lock.

The list head can be concurrently written by drm_bridge_add(), drm_bridge_remove() and __drm_bridge_free() while they operate on neighbouring entries of bridge_list or bridge_lingering_list, all of them under bridge_lock. Hence the unlocked read is a data race, even though it is unlikely to change the outcome of list_empty() in practice.

Perform the check with bridge_lock held.

---
 drivers/gpu/drm/drm_bridge.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 3618cb12483e2..9124cbe1ff889 100644
@@ -558,8 +558,10 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	if (!bridge->container)
 		DRM_WARN("DRM bridge corrupted or not allocated by devm_drm_bridge_alloc()\n");
 
-	if (list_empty(&bridge->list))
-		DRM_WARN("Missing drm_bridge_add() before attach\n");
+	scoped_guard(mutex, &bridge_lock) {
+		if (list_empty(&bridge->list))
+			DRM_WARN("Missing drm_bridge_add() before attach\n");
+	}
 
 	drm_bridge_get(bridge);
 

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Waiman Long <[email protected]>

[ Upstream commit e06b678e3b4e12fdaa0b51a7a5a54abc5dbd6469 ]

After seeing the patch [1] to guard is_in_v2_mode() with RCU, it makes me realize that is_in_v2_mode() may be called in a context where a new cgroup filesystem is being rebound with stale cpuset_cgrp_subsys.root pointer. Avoid this potential UaF situation by adding a new cpuset_v2_mode flag which is set when the cpuset_v2_mode mount option is used. This flag is written into only when cpuset_bind() is being called with a stable cpuset_cgrp_subsys.root value. The is_in_v2_mode() helper is modified to read the new cpuset_v2_mode flag instead of accessing cpuset_cgrp_subsys.root directly.

[1] https://lore.kernel.org/lkml/[email protected]

---
 kernel/cgroup/cpuset.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index 2b86b76dba56b..16c554df4a50e 100644
@@ -151,6 +151,12 @@ static cpumask_var_t	isolated_cpus;		/* CSCB */
  */
 static bool		update_housekeeping;	/* RWCS */
 
+/*
+ * Set if "cpuset_v2_mode" mount option is used
+ * Cached at bind time and not lock protected; accessed via {READ,WRITE}_ONCE
+ */
+static bool		cpuset_v2_mode;
+
 /*
  * Copy of isolated_cpus to be passed to housekeeping_update()
  */

@@ -403,8 +409,7 @@ static inline bool cpuset_v2(void)
  */
 static inline bool is_in_v2_mode(void)
 {
-	return cpuset_v2() ||
-	      (cpuset_cgrp_subsys.root->flags & CGRP_ROOT_CPUSET_V2_MODE);
+	return cpuset_v2() || READ_ONCE(cpuset_v2_mode);
 }
 
 /**

@@ -3602,6 +3607,8 @@ static void cpuset_bind(struct cgroup_subsys_state *root_css)
 	mutex_lock(&cpuset_mutex);
 	spin_lock_irq(&callback_lock);
 
+	WRITE_ONCE(cpuset_v2_mode,
+		   !!(cpuset_cgrp_subsys.root->flags & CGRP_ROOT_CPUSET_V2_MODE));
 	if (is_in_v2_mode()) {
 		cpumask_copy(top_cpuset.cpus_allowed, cpu_possible_mask);
 		cpumask_copy(top_cpuset.effective_xcpus, cpu_possible_mask);

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Oleg Keri <[email protected]>

[ Upstream commit 707ce102521496c67860d81221b20c56d8255f2f ]

If drm_edp_backlight_enable() fails, dp_aux_backlight_update_status() still marks the backlight as enabled. From then on every brightness change only updates the level and the enable is never retried, so the panel stays dark until something blanks and unblanks the backlight.

Return the error instead, so the next update tries to enable again.

---
 drivers/gpu/drm/display/drm_dp_helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index e623ccb4c1d8a..488fd123a01ad 100644
@@ -4634,7 +4634,9 @@ static int dp_aux_backlight_update_status(struct backlight_device *bd)
 
 	if (!backlight_is_blank(bd)) {
 		if (!bl->enabled) {
-			drm_edp_backlight_enable(bl->aux, &bl->info, brightness);
+			ret = drm_edp_backlight_enable(bl->aux, &bl->info, brightness);
+			if (ret)
+				return ret;
 			bl->enabled = true;
 			return 0;
 		}

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Sang-Hoon Choi <[email protected]>

[ Upstream commit 4d69e74034b208588636ecc177e0dd43a7696af3 ]

devm_add_action_or_reset() invokes the cleanup action immediately when registering the action fails. gpio_exar_probe() currently stores the allocated ID in exar_gpio only after registering the action, so that failure makes exar_devm_ida_free() release ID 0. If the allocated ID is nonzero, this leaks that ID and can free ID 0 belonging to another device.

Store the ID before registering the cleanup action so both the immediate failure path and normal device teardown release the correct allocation.

---
 drivers/gpio/gpio-exar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 9053662f18177..f629cc526f3ee 100644
@@ -199,6 +199,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	index = ida_alloc(&ida_index, GFP_KERNEL);
 	if (index < 0)
 		return index;
+	exar_gpio->index = index;
 
 	ret = devm_add_action_or_reset(dev, exar_devm_ida_free, exar_gpio);
 	if (ret)

@@ -214,7 +215,6 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	exar_gpio->gpio_chip.set = exar_set_value;
 	exar_gpio->gpio_chip.base = -1;
 	exar_gpio->gpio_chip.ngpio = ngpios;
-	exar_gpio->index = index;
 	exar_gpio->first_pin = first_pin;
 
 	ret = devm_gpiochip_add_data(dev, &exar_gpio->gpio_chip, exar_gpio);

2.53.0

7.2-stable review patch. If anyone has any objections, please let me know.

------------------

From: Myeonghun Pak <[email protected]>

[ Upstream commit ecce445c6489895463b7ddec2af66338287fee4c ]

aspeed_gfx_load() enables the display clock without checking the return value and never disables it on probe failure or removal. devm_clk_get() only releases the clock reference, leaving the enable unbalanced.

Use devm_clk_get_enabled() to handle enable failures and automatically disable and unprepare the clock on probe failure or removal.

This issue was identified during our ongoing static-analysis research while reviewing kernel code.

---
 drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index d4577663a1f0e..aa36b79d0f6c4 100644
@@ -189,13 +189,12 @@ static int aspeed_gfx_load(struct drm_device *drm)
 	}
 	reset_control_deassert(priv->rst);
 
-	priv->clk = devm_clk_get(drm->dev, NULL);
+	priv->clk = devm_clk_get_enabled(drm->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(&pdev->dev,
-			"missing or invalid clk device tree entry");
+			"failed to get and enable clock\n");
 		return PTR_ERR(priv->clk);
 	}
-	clk_prepare_enable(priv->clk);
 
 	/* Sanitize control registers */
 	writel(0, priv->base + CRT_CTRL1);

2.53.0