FAILED: patch "[PATCH] x86/resctrl: Remove superfluous kernfs_get() calls to prevent" failed to apply to 4.19-stable tree

PATCH In review [email protected] 2 participants 3 messages Nov 30, 2020 – Dec 1, 2020

Subsystems (3)

Stable branch x86 RDT - resource allocation

lore

Share & tools

lore

[email protected]

author maintainer

2020-11-30 08:34 UTC #0 lore raw

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From fd8d9db3559a29fd737bcdb7c4fcbe1940caae34 Mon Sep 17 00:00:00 2001
From: Xiaochen Shen <[email protected]>
Date: Sat, 31 Oct 2020 03:10:53 +0800
Subject: [PATCH] x86/resctrl: Remove superfluous kernfs_get() calls to prevent
refcount leak

Willem reported growing of kernfs_node_cache entries in slabtop when repeatedly creating and removing resctrl subdirectories as well as when repeatedly mounting and unmounting the resctrl filesystem.

On resource group (control as well as monitoring) creation via a mkdir an extra kernfs_node reference is obtained to ensure that the rdtgroup structure remains accessible for the rdtgroup_kn_unlock() calls where it is removed on deletion. The kernfs_node reference count is dropped by kernfs_put() in rdtgroup_kn_unlock().

With the above explaining the need for one kernfs_get()/kernfs_put() pair in resctrl there are more places where a kernfs_node reference is obtained without a corresponding release. The excessive amount of reference count on kernfs nodes will never be dropped to 0 and the kernfs nodes will never be freed in the call paths of rmdir and umount. It leads to reference count leak and kernfs_node_cache memory leak.

Remove the superfluous kernfs_get() calls and expand the existing comments surrounding the remaining kernfs_get()/kernfs_put() pair that remains in use.

Superfluous kernfs_get() calls are removed from two areas:

(1) In call paths of mount and mkdir, when kernfs nodes for "info", "mon_groups" and "mon_data" directories and sub-directories are created, the reference count of newly created kernfs node is set to 1. But after kernfs_create_dir() returns, superfluous kernfs_get() are called to take an additional reference.

(2) kernfs_get() calls in rmdir call paths.

Fixes 17eafd076291 ("x86/intel_rdt: Split resource group removal in two")

Fixes 4af4a88e0c92 ("x86/intel_rdt/cqm: Add mount,umount support")

Fixes f3cbeacaa06e ("x86/intel_rdt/cqm: Add rmdir support")

Fixes d89b7379015f ("x86/intel_rdt/cqm: Add mon_data")

Fixes c7d9aac61311 ("x86/intel_rdt/cqm: Add mkdir support for RDT monitoring")

Fixes 5dc1d5c6bac2 ("x86/intel_rdt: Simplify info and base file lists")

Fixes 60cf5e101fd4 ("x86/intel_rdt: Add mkdir to resctrl file system")

Fixes 4e978d06dedb ("x86/intel_rdt: Add "info" files to resctrl file system")

Reported-by Willem de Bruijn <[email protected]>

Signed-off-by Xiaochen Shen <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

Reviewed-by Reinette Chatre <[email protected]>

Tested-by Willem de Bruijn <[email protected]>

Cc [email protected]

Link https://lkml.kernel.org/r/[email protected] open here

arch/x86/kernel/cpu/resctrl/rdtgroup.c +2 ~~-33~~

index af323e2e3100..2ab1266a5f14 100644
@@ -1769,7 +1769,6 @@ static int rdtgroup_mkdir_info_resdir(struct rdt_resource *r, char *name,
 	if (IS_ERR(kn_subdir))
 		return PTR_ERR(kn_subdir);
 
-	kernfs_get(kn_subdir);
 	ret = rdtgroup_kn_set_ugid(kn_subdir);
 	if (ret)
 		return ret;

@@ -1792,7 +1791,6 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 	kn_info = kernfs_create_dir(parent_kn, "info", parent_kn->mode, NULL);
 	if (IS_ERR(kn_info))
 		return PTR_ERR(kn_info);
-	kernfs_get(kn_info);
 
 	ret = rdtgroup_add_files(kn_info, RF_TOP_INFO);
 	if (ret)

@@ -1813,12 +1811,6 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 			goto out_destroy;
 	}
 
-	/*
-	 * This extra ref will be put in kernfs_remove() and guarantees
-	 * that @rdtgrp->kn is always accessible.
-	 */
-	kernfs_get(kn_info);
-
 	ret = rdtgroup_kn_set_ugid(kn_info);
 	if (ret)
 		goto out_destroy;

@@ -1847,12 +1839,6 @@ mongroup_create_dir(struct kernfs_node *parent_kn, struct rdtgroup *prgrp,
 	if (dest_kn)
 		*dest_kn = kn;
 
-	/*
-	 * This extra ref will be put in kernfs_remove() and guarantees
-	 * that @rdtgrp->kn is always accessible.
-	 */
-	kernfs_get(kn);
-
 	ret = rdtgroup_kn_set_ugid(kn);
 	if (ret)
 		goto out_destroy;

@@ -2139,13 +2125,11 @@ static int rdt_get_tree(struct fs_context *fc)
 					  &kn_mongrp);
 		if (ret < 0)
 			goto out_info;
-		kernfs_get(kn_mongrp);
 
 		ret = mkdir_mondata_all(rdtgroup_default.kn,
 					&rdtgroup_default, &kn_mondata);
 		if (ret < 0)
 			goto out_mongrp;
-		kernfs_get(kn_mondata);
 		rdtgroup_default.mon.mon_data_kn = kn_mondata;
 	}

@@ -2499,11 +2483,6 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
 	if (IS_ERR(kn))
 		return PTR_ERR(kn);
 
-	/*
-	 * This extra ref will be put in kernfs_remove() and guarantees
-	 * that kn is always accessible.
-	 */
-	kernfs_get(kn);
 	ret = rdtgroup_kn_set_ugid(kn);
 	if (ret)
 		goto out_destroy;

@@ -2838,8 +2817,8 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 	/*
 	 * kernfs_remove() will drop the reference count on "kn" which
 	 * will free it. But we still need it to stick around for the
-	 * rdtgroup_kn_unlock(kn} call below. Take one extra reference
-	 * here, which will be dropped inside rdtgroup_kn_unlock().
+	 * rdtgroup_kn_unlock(kn) call. Take one extra reference here,
+	 * which will be dropped inside rdtgroup_kn_unlock().
 	 */
 	kernfs_get(kn);

@@ -3049,11 +3028,6 @@ static int rdtgroup_rmdir_mon(struct kernfs_node *kn, struct rdtgroup *rdtgrp,
 	WARN_ON(list_empty(&prdtgrp->mon.crdtgrp_list));
 	list_del(&rdtgrp->mon.crdtgrp_list);
 
-	/*
-	 * one extra hold on this, will drop when we kfree(rdtgrp)
-	 * in rdtgroup_kn_unlock()
-	 */
-	kernfs_get(kn);
 	kernfs_remove(rdtgrp->kn);
 
 	return 0;

@@ -3065,11 +3039,6 @@ static int rdtgroup_ctrl_remove(struct kernfs_node *kn,
 	rdtgrp->flags = RDT_DELETED;
 	list_del(&rdtgrp->rdtgroup_list);
 
-	/*
-	 * one extra hold on this, will drop when we kfree(rdtgrp)
-	 * in rdtgroup_kn_unlock()
-	 */
-	kernfs_get(kn);
 	kernfs_remove(rdtgrp->kn);
 	return 0;
 }

Xiaochen Shen

2020-11-30 16:57 UTC #1 lore raw

PATCH 1/2 x86/resctrl: Remove superfluous kernfs_get() calls to prevent refcount leak

commit fd8d9db3559a29fd737bcdb7c4fcbe1940caae34 upstream.

Remove the superfluous kernfs_get() calls and expand the existing comments surrounding the remaining kernfs_get()/kernfs_put() pair that remains in use.

Superfluous kernfs_get() calls are removed from two areas:

(2) kernfs_get() calls in rmdir call paths.

Backporting notes:

Since upstream commit fa7d949337cc ("x86/resctrl: Rename and move rdt files to a separate directory"), the file arch/x86/kernel/cpu/intel_rdt_rdtgroup.c has been renamed and moved to arch/x86/kernel/cpu/resctrl/rdtgroup.c.
Apply the change against file arch/x86/kernel/cpu/intel_rdt_rdtgroup.c for older stable trees.

Fixes 17eafd076291 ("x86/intel_rdt: Split resource group removal in two")

Fixes 4af4a88e0c92 ("x86/intel_rdt/cqm: Add mount,umount support")

Fixes f3cbeacaa06e ("x86/intel_rdt/cqm: Add rmdir support")

Fixes d89b7379015f ("x86/intel_rdt/cqm: Add mon_data")

Fixes c7d9aac61311 ("x86/intel_rdt/cqm: Add mkdir support for RDT monitoring")

Fixes 5dc1d5c6bac2 ("x86/intel_rdt: Simplify info and base file lists")

Fixes 60cf5e101fd4 ("x86/intel_rdt: Add mkdir to resctrl file system")

Fixes 4e978d06dedb ("x86/intel_rdt: Add "info" files to resctrl file system")

Reported-by Willem de Bruijn <[email protected]>

Signed-off-by Xiaochen Shen <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

Reviewed-by Reinette Chatre <[email protected]>

Tested-by Willem de Bruijn <[email protected]>

Cc [email protected]

Link https://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 35 ++------------------------------
 1 file changed, 2 insertions(+), 33 deletions(-)

arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +2 ~~-33~~

index e62e416..dc9061a 100644
@@ -1626,7 +1626,6 @@ static int rdtgroup_mkdir_info_resdir(struct rdt_resource *r, char *name,
 	if (IS_ERR(kn_subdir))
 		return PTR_ERR(kn_subdir);
 
-	kernfs_get(kn_subdir);
 	ret = rdtgroup_kn_set_ugid(kn_subdir);
 	if (ret)
 		return ret;

@@ -1649,7 +1648,6 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 	kn_info = kernfs_create_dir(parent_kn, "info", parent_kn->mode, NULL);
 	if (IS_ERR(kn_info))
 		return PTR_ERR(kn_info);
-	kernfs_get(kn_info);
 
 	ret = rdtgroup_add_files(kn_info, RF_TOP_INFO);
 	if (ret)

@@ -1670,12 +1668,6 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 			goto out_destroy;
 	}
 
-	/*
-	 * This extra ref will be put in kernfs_remove() and guarantees
-	 * that @rdtgrp->kn is always accessible.
-	 */
-	kernfs_get(kn_info);
-
 	ret = rdtgroup_kn_set_ugid(kn_info);
 	if (ret)
 		goto out_destroy;

@@ -1704,12 +1696,6 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 	if (dest_kn)
 		*dest_kn = kn;
 
-	/*
-	 * This extra ref will be put in kernfs_remove() and guarantees
-	 * that @rdtgrp->kn is always accessible.
-	 */
-	kernfs_get(kn);
-
 	ret = rdtgroup_kn_set_ugid(kn);
 	if (ret)
 		goto out_destroy;

@@ -2025,7 +2011,6 @@ static struct dentry *rdt_mount(struct file_system_type *fs_type,
 			dentry = ERR_PTR(ret);
 			goto out_info;
 		}
-		kernfs_get(kn_mongrp);
 
 		ret = mkdir_mondata_all(rdtgroup_default.kn,
 					&rdtgroup_default, &kn_mondata);

@@ -2033,7 +2018,6 @@ static struct dentry *rdt_mount(struct file_system_type *fs_type,
 			dentry = ERR_PTR(ret);
 			goto out_mongrp;
 		}
-		kernfs_get(kn_mondata);
 		rdtgroup_default.mon.mon_data_kn = kn_mondata;
 	}

@@ -2326,11 +2310,6 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
 	if (IS_ERR(kn))
 		return PTR_ERR(kn);
 
-	/*
-	 * This extra ref will be put in kernfs_remove() and guarantees
-	 * that kn is always accessible.
-	 */
-	kernfs_get(kn);
 	ret = rdtgroup_kn_set_ugid(kn);
 	if (ret)
 		goto out_destroy;

@@ -2622,8 +2601,8 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 	/*
 	 * kernfs_remove() will drop the reference count on "kn" which
 	 * will free it. But we still need it to stick around for the
-	 * rdtgroup_kn_unlock(kn} call below. Take one extra reference
-	 * here, which will be dropped inside rdtgroup_kn_unlock().
+	 * rdtgroup_kn_unlock(kn) call. Take one extra reference here,
+	 * which will be dropped inside rdtgroup_kn_unlock().
 	 */
 	kernfs_get(kn);

@@ -2838,11 +2817,6 @@ static int rdtgroup_rmdir_mon(struct kernfs_node *kn, struct rdtgroup *rdtgrp,
 	WARN_ON(list_empty(&prdtgrp->mon.crdtgrp_list));
 	list_del(&rdtgrp->mon.crdtgrp_list);
 
-	/*
-	 * one extra hold on this, will drop when we kfree(rdtgrp)
-	 * in rdtgroup_kn_unlock()
-	 */
-	kernfs_get(kn);
 	kernfs_remove(rdtgrp->kn);
 
 	return 0;

@@ -2854,11 +2828,6 @@ static int rdtgroup_ctrl_remove(struct kernfs_node *kn,
 	rdtgrp->flags = RDT_DELETED;
 	list_del(&rdtgrp->rdtgroup_list);
 
-	/*
-	 * one extra hold on this, will drop when we kfree(rdtgrp)
-	 * in rdtgroup_kn_unlock()
-	 */
-	kernfs_get(kn);
 	kernfs_remove(rdtgrp->kn);
 	return 0;
 }

Signature · 1 line

1.8.3.1

Greg KH

author maintainer

2020-12-01 08:26 UTC #2 lore raw

Re: [PATCH 4.19 1/2] x86/resctrl: Remove superfluous kernfs_get() calls to prevent refcount leak

Quoted 2 lines · Xiaochen Shen

commit fd8d9db3559a29fd737bcdb7c4fcbe1940caae34 upstream.

<snip>

All backports now queued up, thanks!

greg k-h