Re: [PATCH] mm: fix hugetlb bug due to user_shm_unlock call (fwd)

Re: [PATCH] mm: fix hugetlb bug due to user_shm_unlock call (fwd)

[Hugh Dickins]

On Fri, 23 Aug 2009, Stefan Huber wrote:
> We have recently detected a kernel oops bug in the hugetlb code. The bug
> is still present in the current linux-2.6 git branch (tested until [1]).
> We have attached a 'git format-patch'-file that solved problem for us.
> The commit message should describe the logic of the bug. Please contact
> me if you have further questions or comments.
> 
> Sincerely,
> Stefan Huber.
> 
> [1]  linux/kernel/git/torvalds/linux-2.6.git (commit
>      429966b8f644dda2afddb4f834a944e9b46a7645)

That's a valuable discovery, thank you for reporting it.

However, though I can well believe that your patch works well for you,
I don't think it's general enough: there is no guarantee that the tests
in can_do_hugetlb_shm() will give the same answer to the user who ends
up calling shm_destroy() as it did once upon a time to the user who
called hugetlb_file_setup().

So, please could you try this alternative patch below, to see if it
passes your testing too, and let us know the result?  I'm sure we'd
like to get a fix into 2.6.31, and into 2.6.30-stable.

Thanks,
Hugh


[PATCH] mm: fix hugetlb bug due to user_shm_unlock call

2.6.30's commit 8a0bdec194c21c8fdef840989d0d7b742bb5d4bc removed
user_shm_lock() calls in hugetlb_file_setup() but left the
user_shm_unlock call in shm_destroy().

In detail:
Assume that can_do_hugetlb_shm() returns true and hence user_shm_lock()
is not called in hugetlb_file_setup(). However, user_shm_unlock() is
called in any case in shm_destroy() and in the following
atomic_dec_and_lock(&up->__count) in free_uid() is executed and if
up->__count gets zero, also cleanup_user_struct() is scheduled.

Note that sched_destroy_user() is empty if CONFIG_USER_SCHED is not set.
However, the ref counter up->__count gets unexpectedly non-positive and
the corresponding structs are freed even though there are live
references to them, resulting in a kernel oops after a lots of
shmget(SHM_HUGETLB)/shmctl(IPC_RMID) cycles and CONFIG_USER_SCHED set.

Reported-by: Stefan Huber <shuber2@gmail.com>
Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Cc: stable@kernel.org
---

 fs/hugetlbfs/inode.c    |   20 ++++++++++++--------
 include/linux/hugetlb.h |    6 ++++--
 ipc/shm.c               |    6 +++---
 3 files changed, 19 insertions(+), 13 deletions(-)

--- 2.6.31-rc7/fs/hugetlbfs/inode.c	2009-06-25 05:18:06.000000000 +0100
+++ linux/fs/hugetlbfs/inode.c	2009-08-24 12:32:01.000000000 +0100
@@ -935,26 +935,28 @@ static int can_do_hugetlb_shm(void)
 	return capable(CAP_IPC_LOCK) || in_group_p(sysctl_hugetlb_shm_group);
 }
 
-struct file *hugetlb_file_setup(const char *name, size_t size, int acctflag)
+struct file *hugetlb_file_setup(const char *name, size_t size, int acctflag,
+						struct user_struct **user)
 {
 	int error = -ENOMEM;
-	int unlock_shm = 0;
 	struct file *file;
 	struct inode *inode;
 	struct dentry *dentry, *root;
 	struct qstr quick_string;
-	struct user_struct *user = current_user();
 
+	*user = NULL;
 	if (!hugetlbfs_vfsmount)
 		return ERR_PTR(-ENOENT);
 
 	if (!can_do_hugetlb_shm()) {
-		if (user_shm_lock(size, user)) {
-			unlock_shm = 1;
+		*user = current_user();
+		if (user_shm_lock(size, *user)) {
 			WARN_ONCE(1,
 			  "Using mlock ulimits for SHM_HUGETLB deprecated\n");
-		} else
+		} else {
+			*user = NULL;
 			return ERR_PTR(-EPERM);
+		}
 	}
 
 	root = hugetlbfs_vfsmount->mnt_root;
@@ -996,8 +998,10 @@ out_inode:
 out_dentry:
 	dput(dentry);
 out_shm_unlock:
-	if (unlock_shm)
-		user_shm_unlock(size, user);
+	if (*user) {
+		user_shm_unlock(size, *user);
+		*user = NULL;
+	}
 	return ERR_PTR(error);
 }
 
--- 2.6.31-rc7/include/linux/hugetlb.h	2009-06-25 05:18:08.000000000 +0100
+++ linux/include/linux/hugetlb.h	2009-08-24 12:32:01.000000000 +0100
@@ -10,6 +10,7 @@
 #include <asm/tlbflush.h>
 
 struct ctl_table;
+struct user_struct;
 
 int PageHuge(struct page *page);
 
@@ -146,7 +147,8 @@ static inline struct hugetlbfs_sb_info *
 
 extern const struct file_operations hugetlbfs_file_operations;
 extern struct vm_operations_struct hugetlb_vm_ops;
-struct file *hugetlb_file_setup(const char *name, size_t, int);
+struct file *hugetlb_file_setup(const char *name, size_t size, int acct,
+						struct user_struct **user);
 int hugetlb_get_quota(struct address_space *mapping, long delta);
 void hugetlb_put_quota(struct address_space *mapping, long delta);
 
@@ -168,7 +170,7 @@ static inline void set_file_hugepages(st
 
 #define is_file_hugepages(file)			0
 #define set_file_hugepages(file)		BUG()
-#define hugetlb_file_setup(name,size,acctflag)	ERR_PTR(-ENOSYS)
+#define hugetlb_file_setup(name,size,acct,user)	ERR_PTR(-ENOSYS)
 
 #endif /* !CONFIG_HUGETLBFS */
 
--- 2.6.31-rc7/ipc/shm.c	2009-06-25 05:18:09.000000000 +0100
+++ linux/ipc/shm.c	2009-08-24 12:32:01.000000000 +0100
@@ -174,7 +174,7 @@ static void shm_destroy(struct ipc_names
 	shm_unlock(shp);
 	if (!is_file_hugepages(shp->shm_file))
 		shmem_lock(shp->shm_file, 0, shp->mlock_user);
-	else
+	else if (shp->mlock_user)
 		user_shm_unlock(shp->shm_file->f_path.dentry->d_inode->i_size,
 						shp->mlock_user);
 	fput (shp->shm_file);
@@ -369,8 +369,8 @@ static int newseg(struct ipc_namespace *
 		/* hugetlb_file_setup applies strict accounting */
 		if (shmflg & SHM_NORESERVE)
 			acctflag = VM_NORESERVE;
-		file = hugetlb_file_setup(name, size, acctflag);
-		shp->mlock_user = current_user();
+		file = hugetlb_file_setup(name, size, acctflag,
+							&shp->mlock_user);
 	} else {
 		/*
 		 * Do not allow no accounting for OVERCOMMIT_NEVER, even

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix hugetlb bug due to user_shm_unlock call (fwd)

[Stefan Huber]

[-- Attachment #1: Type: text/plain, Size: 635 bytes --]

> However, though I can well believe that your patch works well for you,
> I don't think it's general enough: there is no guarantee that the tests
> in can_do_hugetlb_shm() will give the same answer to the user who ends
> up calling shm_destroy() as it did once upon a time to the user who
> called hugetlb_file_setup().
> 
> So, please could you try this alternative patch below, to see if it
> passes your testing too, and let us know the result?  I'm sure we'd
> like to get a fix into 2.6.31, and into 2.6.30-stable.


Yes, your observation is right and your modified patch works good for
me.

So long
Stefan




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]

[PATCH] mm: fix hugetlb bug due to user_shm_unlock call

[Hugh Dickins]

2.6.30's commit 8a0bdec194c21c8fdef840989d0d7b742bb5d4bc removed
user_shm_lock() calls in hugetlb_file_setup() but left the
user_shm_unlock call in shm_destroy().

In detail:
Assume that can_do_hugetlb_shm() returns true and hence user_shm_lock()
is not called in hugetlb_file_setup(). However, user_shm_unlock() is
called in any case in shm_destroy() and in the following
atomic_dec_and_lock(&up->__count) in free_uid() is executed and if
up->__count gets zero, also cleanup_user_struct() is scheduled.

Note that sched_destroy_user() is empty if CONFIG_USER_SCHED is not set.
However, the ref counter up->__count gets unexpectedly non-positive and
the corresponding structs are freed even though there are live
references to them, resulting in a kernel oops after a lots of
shmget(SHM_HUGETLB)/shmctl(IPC_RMID) cycles and CONFIG_USER_SCHED set.

Hugh changed Stefan's suggested patch: can_do_hugetlb_shm() at the
time of shm_destroy() may give a different answer from at the time
of hugetlb_file_setup().  And fixed newseg()'s no_id error path,
which has missed user_shm_unlock() ever since it came in 2.6.9.

Reported-by: Stefan Huber <shuber2@gmail.com>
Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Tested-by: Stefan Huber <shuber2@gmail.com>
Cc: stable@kernel.org
---
Stefan, thanks a lot for reporting and testing and reporting back.
No need for you to retest, but in preparing to send this out, I've
noticed another error here, dating back to 2.6.9 (see comment above),
so added in the fix to that (rare case) too.

 fs/hugetlbfs/inode.c    |   20 ++++++++++++--------
 include/linux/hugetlb.h |    6 ++++--
 ipc/shm.c               |    8 +++++---
 3 files changed, 21 insertions(+), 13 deletions(-)

--- 2.6.31-rc7/fs/hugetlbfs/inode.c	2009-06-25 05:18:06.000000000 +0100
+++ linux/fs/hugetlbfs/inode.c	2009-08-24 12:32:01.000000000 +0100
@@ -935,26 +935,28 @@ static int can_do_hugetlb_shm(void)
 	return capable(CAP_IPC_LOCK) || in_group_p(sysctl_hugetlb_shm_group);
 }
 
-struct file *hugetlb_file_setup(const char *name, size_t size, int acctflag)
+struct file *hugetlb_file_setup(const char *name, size_t size, int acctflag,
+						struct user_struct **user)
 {
 	int error = -ENOMEM;
-	int unlock_shm = 0;
 	struct file *file;
 	struct inode *inode;
 	struct dentry *dentry, *root;
 	struct qstr quick_string;
-	struct user_struct *user = current_user();
 
+	*user = NULL;
 	if (!hugetlbfs_vfsmount)
 		return ERR_PTR(-ENOENT);
 
 	if (!can_do_hugetlb_shm()) {
-		if (user_shm_lock(size, user)) {
-			unlock_shm = 1;
+		*user = current_user();
+		if (user_shm_lock(size, *user)) {
 			WARN_ONCE(1,
 			  "Using mlock ulimits for SHM_HUGETLB deprecated\n");
-		} else
+		} else {
+			*user = NULL;
 			return ERR_PTR(-EPERM);
+		}
 	}
 
 	root = hugetlbfs_vfsmount->mnt_root;
@@ -996,8 +998,10 @@ out_inode:
 out_dentry:
 	dput(dentry);
 out_shm_unlock:
-	if (unlock_shm)
-		user_shm_unlock(size, user);
+	if (*user) {
+		user_shm_unlock(size, *user);
+		*user = NULL;
+	}
 	return ERR_PTR(error);
 }
 
--- 2.6.31-rc7/include/linux/hugetlb.h	2009-06-25 05:18:08.000000000 +0100
+++ linux/include/linux/hugetlb.h	2009-08-24 12:32:01.000000000 +0100
@@ -10,6 +10,7 @@
 #include <asm/tlbflush.h>
 
 struct ctl_table;
+struct user_struct;
 
 int PageHuge(struct page *page);
 
@@ -146,7 +147,8 @@ static inline struct hugetlbfs_sb_info *
 
 extern const struct file_operations hugetlbfs_file_operations;
 extern struct vm_operations_struct hugetlb_vm_ops;
-struct file *hugetlb_file_setup(const char *name, size_t, int);
+struct file *hugetlb_file_setup(const char *name, size_t size, int acct,
+						struct user_struct **user);
 int hugetlb_get_quota(struct address_space *mapping, long delta);
 void hugetlb_put_quota(struct address_space *mapping, long delta);
 
@@ -168,7 +170,7 @@ static inline void set_file_hugepages(st
 
 #define is_file_hugepages(file)			0
 #define set_file_hugepages(file)		BUG()
-#define hugetlb_file_setup(name,size,acctflag)	ERR_PTR(-ENOSYS)
+#define hugetlb_file_setup(name,size,acct,user)	ERR_PTR(-ENOSYS)
 
 #endif /* !CONFIG_HUGETLBFS */
 
--- 2.6.31-rc7/ipc/shm.c	2009-06-25 05:18:09.000000000 +0100
+++ linux/ipc/shm.c	2009-08-24 16:06:30.000000000 +0100
@@ -174,7 +174,7 @@ static void shm_destroy(struct ipc_names
 	shm_unlock(shp);
 	if (!is_file_hugepages(shp->shm_file))
 		shmem_lock(shp->shm_file, 0, shp->mlock_user);
-	else
+	else if (shp->mlock_user)
 		user_shm_unlock(shp->shm_file->f_path.dentry->d_inode->i_size,
 						shp->mlock_user);
 	fput (shp->shm_file);
@@ -369,8 +369,8 @@ static int newseg(struct ipc_namespace *
 		/* hugetlb_file_setup applies strict accounting */
 		if (shmflg & SHM_NORESERVE)
 			acctflag = VM_NORESERVE;
-		file = hugetlb_file_setup(name, size, acctflag);
-		shp->mlock_user = current_user();
+		file = hugetlb_file_setup(name, size, acctflag,
+							&shp->mlock_user);
 	} else {
 		/*
 		 * Do not allow no accounting for OVERCOMMIT_NEVER, even
@@ -410,6 +410,8 @@ static int newseg(struct ipc_namespace *
 	return error;
 
 no_id:
+	if (shp->mlock_user)	/* shmflg & SHM_HUGETLB case */
+		user_shm_unlock(size, shp->mlock_user);
 	fput(file);
 no_file:
 	security_shm_free(shp);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix hugetlb bug due to user_shm_unlock call

[Mel Gorman]

On Mon, Aug 24, 2009 at 04:30:28PM +0100, Hugh Dickins wrote:
> 2.6.30's commit 8a0bdec194c21c8fdef840989d0d7b742bb5d4bc removed
> user_shm_lock() calls in hugetlb_file_setup() but left the
> user_shm_unlock call in shm_destroy().
> 
> In detail:
> Assume that can_do_hugetlb_shm() returns true and hence user_shm_lock()
> is not called in hugetlb_file_setup(). However, user_shm_unlock() is
> called in any case in shm_destroy() and in the following
> atomic_dec_and_lock(&up->__count) in free_uid() is executed and if
> up->__count gets zero, also cleanup_user_struct() is scheduled.
> 
> Note that sched_destroy_user() is empty if CONFIG_USER_SCHED is not set.
> However, the ref counter up->__count gets unexpectedly non-positive and
> the corresponding structs are freed even though there are live
> references to them, resulting in a kernel oops after a lots of
> shmget(SHM_HUGETLB)/shmctl(IPC_RMID) cycles and CONFIG_USER_SCHED set.
> 
> Hugh changed Stefan's suggested patch: can_do_hugetlb_shm() at the
> time of shm_destroy() may give a different answer from at the time
> of hugetlb_file_setup().  And fixed newseg()'s no_id error path,
> which has missed user_shm_unlock() ever since it came in 2.6.9.
> 
> Reported-by: Stefan Huber <shuber2@gmail.com>
> Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> Tested-by: Stefan Huber <shuber2@gmail.com>
> Cc: stable@kernel.org

Acked-by: Mel Gorman <mel@csn.ul.ie>

Thanks Hugh.

> ---
> Stefan, thanks a lot for reporting and testing and reporting back.
> No need for you to retest, but in preparing to send this out, I've
> noticed another error here, dating back to 2.6.9 (see comment above),
> so added in the fix to that (rare case) too.
> 
>  fs/hugetlbfs/inode.c    |   20 ++++++++++++--------
>  include/linux/hugetlb.h |    6 ++++--
>  ipc/shm.c               |    8 +++++---
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> --- 2.6.31-rc7/fs/hugetlbfs/inode.c	2009-06-25 05:18:06.000000000 +0100
> +++ linux/fs/hugetlbfs/inode.c	2009-08-24 12:32:01.000000000 +0100
> @@ -935,26 +935,28 @@ static int can_do_hugetlb_shm(void)
>  	return capable(CAP_IPC_LOCK) || in_group_p(sysctl_hugetlb_shm_group);
>  }
>  
> -struct file *hugetlb_file_setup(const char *name, size_t size, int acctflag)
> +struct file *hugetlb_file_setup(const char *name, size_t size, int acctflag,
> +						struct user_struct **user)
>  {
>  	int error = -ENOMEM;
> -	int unlock_shm = 0;
>  	struct file *file;
>  	struct inode *inode;
>  	struct dentry *dentry, *root;
>  	struct qstr quick_string;
> -	struct user_struct *user = current_user();
>  
> +	*user = NULL;
>  	if (!hugetlbfs_vfsmount)
>  		return ERR_PTR(-ENOENT);
>  
>  	if (!can_do_hugetlb_shm()) {
> -		if (user_shm_lock(size, user)) {
> -			unlock_shm = 1;
> +		*user = current_user();
> +		if (user_shm_lock(size, *user)) {
>  			WARN_ONCE(1,
>  			  "Using mlock ulimits for SHM_HUGETLB deprecated\n");
> -		} else
> +		} else {
> +			*user = NULL;
>  			return ERR_PTR(-EPERM);
> +		}
>  	}
>  
>  	root = hugetlbfs_vfsmount->mnt_root;
> @@ -996,8 +998,10 @@ out_inode:
>  out_dentry:
>  	dput(dentry);
>  out_shm_unlock:
> -	if (unlock_shm)
> -		user_shm_unlock(size, user);
> +	if (*user) {
> +		user_shm_unlock(size, *user);
> +		*user = NULL;
> +	}
>  	return ERR_PTR(error);
>  }
>  
> --- 2.6.31-rc7/include/linux/hugetlb.h	2009-06-25 05:18:08.000000000 +0100
> +++ linux/include/linux/hugetlb.h	2009-08-24 12:32:01.000000000 +0100
> @@ -10,6 +10,7 @@
>  #include <asm/tlbflush.h>
>  
>  struct ctl_table;
> +struct user_struct;
>  
>  int PageHuge(struct page *page);
>  
> @@ -146,7 +147,8 @@ static inline struct hugetlbfs_sb_info *
>  
>  extern const struct file_operations hugetlbfs_file_operations;
>  extern struct vm_operations_struct hugetlb_vm_ops;
> -struct file *hugetlb_file_setup(const char *name, size_t, int);
> +struct file *hugetlb_file_setup(const char *name, size_t size, int acct,
> +						struct user_struct **user);
>  int hugetlb_get_quota(struct address_space *mapping, long delta);
>  void hugetlb_put_quota(struct address_space *mapping, long delta);
>  
> @@ -168,7 +170,7 @@ static inline void set_file_hugepages(st
>  
>  #define is_file_hugepages(file)			0
>  #define set_file_hugepages(file)		BUG()
> -#define hugetlb_file_setup(name,size,acctflag)	ERR_PTR(-ENOSYS)
> +#define hugetlb_file_setup(name,size,acct,user)	ERR_PTR(-ENOSYS)
>  
>  #endif /* !CONFIG_HUGETLBFS */
>  
> --- 2.6.31-rc7/ipc/shm.c	2009-06-25 05:18:09.000000000 +0100
> +++ linux/ipc/shm.c	2009-08-24 16:06:30.000000000 +0100
> @@ -174,7 +174,7 @@ static void shm_destroy(struct ipc_names
>  	shm_unlock(shp);
>  	if (!is_file_hugepages(shp->shm_file))
>  		shmem_lock(shp->shm_file, 0, shp->mlock_user);
> -	else
> +	else if (shp->mlock_user)
>  		user_shm_unlock(shp->shm_file->f_path.dentry->d_inode->i_size,
>  						shp->mlock_user);
>  	fput (shp->shm_file);
> @@ -369,8 +369,8 @@ static int newseg(struct ipc_namespace *
>  		/* hugetlb_file_setup applies strict accounting */
>  		if (shmflg & SHM_NORESERVE)
>  			acctflag = VM_NORESERVE;
> -		file = hugetlb_file_setup(name, size, acctflag);
> -		shp->mlock_user = current_user();
> +		file = hugetlb_file_setup(name, size, acctflag,
> +							&shp->mlock_user);
>  	} else {
>  		/*
>  		 * Do not allow no accounting for OVERCOMMIT_NEVER, even
> @@ -410,6 +410,8 @@ static int newseg(struct ipc_namespace *
>  	return error;
>  
>  no_id:
> +	if (shp->mlock_user)	/* shmflg & SHM_HUGETLB case */
> +		user_shm_unlock(size, shp->mlock_user);
>  	fput(file);
>  no_file:
>  	security_shm_free(shp);
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix hugetlb bug due to user_shm_unlock call

[Mike Frysinger]

On Mon, Aug 24, 2009 at 11:30, Hugh Dickins wrote:
> --- 2.6.31-rc7/ipc/shm.c        2009-06-25 05:18:09.000000000 +0100
> +++ linux/ipc/shm.c     2009-08-24 16:06:30.000000000 +0100
> @@ -174,7 +174,7 @@ static void shm_destroy(struct ipc_names
>        shm_unlock(shp);
>        if (!is_file_hugepages(shp->shm_file))
>                shmem_lock(shp->shm_file, 0, shp->mlock_user);
> -       else
> +       else if (shp->mlock_user)
>                user_shm_unlock(shp->shm_file->f_path.dentry->d_inode->i_size,
>                                                shp->mlock_user);
>        fput (shp->shm_file);
> @@ -369,8 +369,8 @@ static int newseg(struct ipc_namespace *
>                /* hugetlb_file_setup applies strict accounting */
>                if (shmflg & SHM_NORESERVE)
>                        acctflag = VM_NORESERVE;
> -               file = hugetlb_file_setup(name, size, acctflag);
> -               shp->mlock_user = current_user();
> +               file = hugetlb_file_setup(name, size, acctflag,
> +                                                       &shp->mlock_user);
>        } else {
>                /*
>                 * Do not allow no accounting for OVERCOMMIT_NEVER, even
> @@ -410,6 +410,8 @@ static int newseg(struct ipc_namespace *
>        return error;
>
>  no_id:
> +       if (shp->mlock_user)    /* shmflg & SHM_HUGETLB case */
> +               user_shm_unlock(size, shp->mlock_user);
>        fput(file);
>  no_file:
>        security_shm_free(shp);

this breaks on no-mmu systems due to user_shm_unlock() being
mmu-specific.  normally gcc is smart enough to do dead code culling so
it hasnt caused problems, but not here.  hugetlb support is not
available on no-mmu systems, so the stubbed hugepage functions prevent
calls to user_shm_unlock() and such, but here gcc cant figure it out:

static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
{
...
    shp->mlock_user = NULL;
...
    if (shmflg & SHM_HUGETLB) {
        /* hugetlb_file_setup applies strict accounting */
        if (shmflg & SHM_NORESERVE)
            acctflag = VM_NORESERVE;
        file = hugetlb_file_setup(name, size, acctflag,
                            &shp->mlock_user);
...
    id = ipc_addid(&shm_ids(ns), &shp->shm_perm, ns->shm_ctlmni);
    if (id < 0) {
        error = id;
        goto no_id;
    }
...
no_id:
    if (shp->mlock_user)    /* shmflg & SHM_HUGETLB case */
        user_shm_unlock(size, shp->mlock_user);
...

hugetlb_file_setup() expands to nothing and so mlock_user will never
come back from NULL, but gcc still emits a reference to
user_shm_unlock() in the error path.  perhaps the best thing here is
to just add an #ifdef ?
 no_id:
+#ifdef CONFIG_HUGETLB_PAGE
+    /* gcc isn't smart enough to see that mlock_user goes non-NULL
only by hugetlb */
    if (shp->mlock_user)    /* shmflg & SHM_HUGETLB case */
        user_shm_unlock(size, shp->mlock_user);
+#endif
-mike

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix hugetlb bug due to user_shm_unlock call

[Hugh Dickins]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1477 bytes --]

On Fri, 11 Sep 2009, Mike Frysinger wrote:
> On Mon, Aug 24, 2009 at 11:30, Hugh Dickins wrote:
> >
> >  no_id:
> > +       if (shp->mlock_user)    /* shmflg & SHM_HUGETLB case */
> > +               user_shm_unlock(size, shp->mlock_user);
> >        fput(file);
> >  no_file:
> >        security_shm_free(shp);
> 
> this breaks on no-mmu systems due to user_shm_unlock() being
> mmu-specific.  normally gcc is smart enough to do dead code culling so
> it hasnt caused problems, but not here.  hugetlb support is not
> available on no-mmu systems, so the stubbed hugepage functions prevent
> calls to user_shm_unlock() and such, but here gcc cant figure it out:
> 
...
> 
> hugetlb_file_setup() expands to nothing and so mlock_user will never
> come back from NULL, but gcc still emits a reference to
> user_shm_unlock() in the error path.  perhaps the best thing here is
> to just add an #ifdef ?
>  no_id:
> +#ifdef CONFIG_HUGETLB_PAGE
> +    /* gcc isn't smart enough to see that mlock_user goes non-NULL
> only by hugetlb */
>     if (shp->mlock_user)    /* shmflg & SHM_HUGETLB case */
>         user_shm_unlock(size, shp->mlock_user);
> +#endif

Many thanks for reporting that, Mike.
Sorry, I've messed up both 2.6.31 final and 2.6.30.6 stable.
My preference is to avoid the #ifdef and use precisely the same
optimization technique as is working for it elsewhere.
Patch follows immediately in separate mail.

Hugh

[PATCH] fix undefined reference to user_shm_unlock

[Hugh Dickins]

My 353d5c30c666580347515da609dd74a2b8e9b828 "mm: fix hugetlb bug due to
user_shm_unlock call" broke the CONFIG_SYSVIPC !CONFIG_MMU build of both
2.6.31 and 2.6.30.6: "undefined reference to `user_shm_unlock'".

gcc didn't understand my comment! so couldn't figure out to optimize
away user_shm_unlock() from the error path in the hugetlb-less case,
as it does elsewhere.  Help it to do so, in a language it understands.

Reported-by: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Cc: stable@kernel.org
---

 ipc/shm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 2.6.31/ipc/shm.c	2009-09-09 23:13:59.000000000 +0100
+++ linux/ipc/shm.c	2009-09-12 11:27:00.000000000 +0100
@@ -410,7 +410,7 @@ static int newseg(struct ipc_namespace *
 	return error;
 
 no_id:
-	if (shp->mlock_user)	/* shmflg & SHM_HUGETLB case */
+	if (is_file_hugepages(file) && shp->mlock_user)
 		user_shm_unlock(size, shp->mlock_user);
 	fput(file);
 no_file:

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] fix undefined reference to user_shm_unlock

[Mike Frysinger]

On Sat, Sep 12, 2009 at 07:21, Hugh Dickins wrote:
> My 353d5c30c666580347515da609dd74a2b8e9b828 "mm: fix hugetlb bug due to
> user_shm_unlock call" broke the CONFIG_SYSVIPC !CONFIG_MMU build of both
> 2.6.31 and 2.6.30.6: "undefined reference to `user_shm_unlock'".
>
> gcc didn't understand my comment! so couldn't figure out to optimize
> away user_shm_unlock() from the error path in the hugetlb-less case,
> as it does elsewhere.  Help it to do so, in a language it understands.

thanks, this works for me

> Cc: stable@kernel.org

should go into 2.6.30.7 and 2.6.31.1
-mike

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>