* [PATCH] mm: fix hugetlb bug due to user_shm_unlock call
@ 2009-08-24 15:30 ` Hugh Dickins
0 siblings, 0 replies; 15+ messages in thread
From: Hugh Dickins @ 2009-08-24 15:30 UTC (permalink / raw)
To: Linus Torvalds
Cc: Stefan Huber, Andrew Morton, Peter Meerwald, James Morris,
William Irwin, Mel Gorman, Ravikiran G Thirumalai, linux-kernel,
linux-mm
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>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] mm: fix hugetlb bug due to user_shm_unlock call
2009-08-24 15:30 ` Hugh Dickins
@ 2009-08-25 7:36 ` Mel Gorman
-1 siblings, 0 replies; 15+ messages in thread
From: Mel Gorman @ 2009-08-25 7:36 UTC (permalink / raw)
To: Hugh Dickins
Cc: Linus Torvalds, Stefan Huber, Andrew Morton, Peter Meerwald,
James Morris, William Irwin, Ravikiran G Thirumalai, linux-kernel,
linux-mm
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] mm: fix hugetlb bug due to user_shm_unlock call
@ 2009-08-25 7:36 ` Mel Gorman
0 siblings, 0 replies; 15+ messages in thread
From: Mel Gorman @ 2009-08-25 7:36 UTC (permalink / raw)
To: Hugh Dickins
Cc: Linus Torvalds, Stefan Huber, Andrew Morton, Peter Meerwald,
James Morris, William Irwin, Ravikiran G Thirumalai, linux-kernel,
linux-mm
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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] mm: fix hugetlb bug due to user_shm_unlock call
2009-08-24 15:30 ` Hugh Dickins
@ 2009-09-11 14:03 ` Mike Frysinger
-1 siblings, 0 replies; 15+ messages in thread
From: Mike Frysinger @ 2009-09-11 14:03 UTC (permalink / raw)
To: Hugh Dickins
Cc: Linus Torvalds, Stefan Huber, Andrew Morton, Peter Meerwald,
James Morris, William Irwin, Mel Gorman, Ravikiran G Thirumalai,
linux-kernel, linux-mm
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] mm: fix hugetlb bug due to user_shm_unlock call
@ 2009-09-11 14:03 ` Mike Frysinger
0 siblings, 0 replies; 15+ messages in thread
From: Mike Frysinger @ 2009-09-11 14:03 UTC (permalink / raw)
To: Hugh Dickins
Cc: Linus Torvalds, Stefan Huber, Andrew Morton, Peter Meerwald,
James Morris, William Irwin, Mel Gorman, Ravikiran G Thirumalai,
linux-kernel, linux-mm
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>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] mm: fix hugetlb bug due to user_shm_unlock call
2009-09-11 14:03 ` Mike Frysinger
(?)
@ 2009-09-12 11:17 ` Hugh Dickins
2009-09-12 11:21 ` Hugh Dickins
-1 siblings, 1 reply; 15+ messages in thread
From: Hugh Dickins @ 2009-09-12 11:17 UTC (permalink / raw)
To: Mike Frysinger
Cc: Linus Torvalds, Stefan Huber, Andrew Morton, Peter Meerwald,
James Morris, William Irwin, Mel Gorman, Ravikiran G Thirumalai,
linux-kernel, linux-mm
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] fix undefined reference to user_shm_unlock
2009-09-12 11:17 ` Hugh Dickins
@ 2009-09-12 11:21 ` Hugh Dickins
0 siblings, 0 replies; 15+ messages in thread
From: Hugh Dickins @ 2009-09-12 11:21 UTC (permalink / raw)
To: Linus Torvalds
Cc: Mike Frysinger, Stefan Huber, Andrew Morton, Peter Meerwald,
James Morris, William Irwin, Mel Gorman, Ravikiran G Thirumalai,
linux-kernel, linux-mm, stable
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:
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] fix undefined reference to user_shm_unlock
@ 2009-09-12 11:21 ` Hugh Dickins
0 siblings, 0 replies; 15+ messages in thread
From: Hugh Dickins @ 2009-09-12 11:21 UTC (permalink / raw)
To: Linus Torvalds
Cc: Mike Frysinger, Stefan Huber, Andrew Morton, Peter Meerwald,
James Morris, William Irwin, Mel Gorman, Ravikiran G Thirumalai,
linux-kernel, linux-mm, stable
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>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] fix undefined reference to user_shm_unlock
2009-09-12 11:21 ` Hugh Dickins
@ 2009-09-12 11:41 ` Mike Frysinger
-1 siblings, 0 replies; 15+ messages in thread
From: Mike Frysinger @ 2009-09-12 11:41 UTC (permalink / raw)
To: Hugh Dickins
Cc: Linus Torvalds, Stefan Huber, Andrew Morton, Peter Meerwald,
James Morris, William Irwin, Mel Gorman, Ravikiran G Thirumalai,
linux-kernel, linux-mm, stable
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] fix undefined reference to user_shm_unlock
@ 2009-09-12 11:41 ` Mike Frysinger
0 siblings, 0 replies; 15+ messages in thread
From: Mike Frysinger @ 2009-09-12 11:41 UTC (permalink / raw)
To: Hugh Dickins
Cc: Linus Torvalds, Stefan Huber, Andrew Morton, Peter Meerwald,
James Morris, William Irwin, Mel Gorman, Ravikiran G Thirumalai,
linux-kernel, linux-mm, stable
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>
^ permalink raw reply [flat|nested] 15+ messages in thread