Re: [PATCH] shm: Fix the locking and cleanup error handling in do_shmat.
On Thu, 2007-02-15 at 22:34 -0500, Eric W. Biederman wrote: > > drivers/video/Kconfig:1606:warning: 'select' used by config symbol > 'FB_PS3' > > refer to undefined symbol 'PS3_PS3AV' > > /mnt/md0/devel/linux-mm/ipc/shm.c: In function 'do_shmat': > > /mnt/md0/devel/linux-mm/ipc/shm.c:945: warning: passing argument 1 > of 'IS_ERR' > > makes pointer from integer without a cast > > /mnt/md0/devel/linux-mm/ipc/shm.c:946: warning: passing argument 1 > of 'PTR_ERR' > > makes pointer from integer without a cast > > /mnt/md0/devel/linux-mm/ipc/shm.c:931: warning: label 'out_nattch' > defined but > > not used > > /mnt/md0/devel/linux-mm/ipc/shm.c:890: error: label 'out_put_path' > used but not > > defined > > make[2]: *** [ipc/shm.o] Error 1 > > make[1]: *** [ipc] Error 2 > > make: *** [_all] Error 2 > > Definitely some weird patch application problem. > > All of the calls to IS_ERR and PTR_ERR should have been removed. > Michal since it didn't seem to blow up when Andrew applied it I'm > going to assume the problem is on your end for now. > > Eric > Hi Eric, I am also troubling with the incorrect "nattch" value in do_shmat(). Actually, I found this bugs when I do some LTP testing on blackfin-uClinux platform. The "nattch" value returned from shmctl() system call is wrong. So I think your patch can solve this bug. But which version Linux kernel is your patch applying for? I want to do some test on my blackfin-uClinux 2.6.20 platform. Thanks -Bryan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] shm: Fix the locking and cleanup error handling in do_shmat.
> drivers/video/Kconfig:1606:warning: 'select' used by config symbol 'FB_PS3' > refer to undefined symbol 'PS3_PS3AV' > /mnt/md0/devel/linux-mm/ipc/shm.c: In function 'do_shmat': > /mnt/md0/devel/linux-mm/ipc/shm.c:945: warning: passing argument 1 of 'IS_ERR' > makes pointer from integer without a cast > /mnt/md0/devel/linux-mm/ipc/shm.c:946: warning: passing argument 1 of > 'PTR_ERR' > makes pointer from integer without a cast > /mnt/md0/devel/linux-mm/ipc/shm.c:931: warning: label 'out_nattch' defined but > not used > /mnt/md0/devel/linux-mm/ipc/shm.c:890: error: label 'out_put_path' used but > not > defined > make[2]: *** [ipc/shm.o] Error 1 > make[1]: *** [ipc] Error 2 > make: *** [_all] Error 2 Definitely some weird patch application problem. All of the calls to IS_ERR and PTR_ERR should have been removed. Michal since it didn't seem to blow up when Andrew applied it I'm going to assume the problem is on your end for now. Eric - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] shm: Fix the locking and cleanup error handling in do_shmat.
Eric W. Biederman napisał(a):
> When enhancing do_shmat I forgot to take into account that shm_lock
> is a spinlock, and was allocating memory with the lock held.
>
> This patch fixes that by grabbing a reference to the dentry and
> mounts of shm_file before we drop the shm_lock and then performing
> the memory allocations.
>
> This is also a bit of a general scrub on the error handling.
> Everything is now forced through the single return statement
> for clarity, and the handling of the return address now uses
> fewer casts.
>
> Signed-off-by: Eric W. Biederman <[EMAIL PROTECTED]>
> ---
> ipc/shm.c | 56
> 1 files changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index e0b6544..26b935b 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -815,15 +815,16 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg, ulong *raddr)
> unsigned long flags;
> unsigned long prot;
> int acc_mode;
> - void *user_addr;
> + unsigned long user_addr;
> struct ipc_namespace *ns;
> struct shm_file_data *sfd;
> + struct path path;
> mode_t f_mode;
>
> - if (shmid < 0) {
> - err = -EINVAL;
> + err = -EINVAL;
> + if (shmid < 0)
> goto out;
> - } else if ((addr = (ulong)shmaddr)) {
> + else if ((addr = (ulong)shmaddr)) {
> if (addr & (SHMLBA-1)) {
> if (shmflg & SHM_RND)
> addr &= ~(SHMLBA-1); /* round down */
> @@ -831,12 +832,12 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg, ulong *raddr)
> #ifndef __ARCH_FORCE_SHMLBA
> if (addr & ~PAGE_MASK)
> #endif
> - return -EINVAL;
> + goto out;
> }
> flags = MAP_SHARED | MAP_FIXED;
> } else {
> if ((shmflg & SHM_REMAP))
> - return -EINVAL;
> + goto out;
>
> flags = MAP_SHARED;
> }
> @@ -860,7 +861,6 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg, ulong *raddr)
>* additional creator id...
>*/
> ns = current->nsproxy->ipc_ns;
> - err = -EINVAL;
> shp = shm_lock(ns, shmid);
> if(shp == NULL)
> goto out;
> @@ -877,19 +877,25 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg, ulong *raddr)
> if (err)
> goto out_unlock;
>
> + path.dentry = dget(shp->shm_file->f_path.dentry);
> + path.mnt= mntget(shp->shm_file->f_path.mnt);
> + shp->shm_nattch++;
> + size = i_size_read(path.dentry->d_inode);
> + shm_unlock(shp);
> +
> err = -ENOMEM;
> sfd = kzalloc(sizeof(*sfd), GFP_KERNEL);
> if (!sfd)
> - goto out_unlock;
> + goto out_put_path;
drivers/video/Kconfig:1606:warning: 'select' used by config symbol 'FB_PS3'
refer to undefined symbol 'PS3_PS3AV'
/mnt/md0/devel/linux-mm/ipc/shm.c: In function 'do_shmat':
/mnt/md0/devel/linux-mm/ipc/shm.c:945: warning: passing argument 1 of 'IS_ERR'
makes pointer from integer without a cast
/mnt/md0/devel/linux-mm/ipc/shm.c:946: warning: passing argument 1 of 'PTR_ERR'
makes pointer from integer without a cast
/mnt/md0/devel/linux-mm/ipc/shm.c:931: warning: label 'out_nattch' defined but
not used
/mnt/md0/devel/linux-mm/ipc/shm.c:890: error: label 'out_put_path' used but not
defined
make[2]: *** [ipc/shm.o] Error 1
make[1]: *** [ipc] Error 2
make: *** [_all] Error 2
>
> + err = -ENOMEM;
> file = get_empty_filp();
> if (!file)
> goto out_free;
>
> file->f_op = &shm_file_operations;
> file->private_data = sfd;
> - file->f_path.dentry = dget(shp->shm_file->f_path.dentry);
> - file->f_path.mnt = mntget(shp->shm_file->f_path.mnt);
> + file->f_path = path;
> file->f_mapping = shp->shm_file->f_mapping;
> file->f_mode = f_mode;
> sfd->id = shp->id;
> @@ -897,13 +903,9 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg, ulong *raddr)
> sfd->file = shp->shm_file;
> sfd->vm_ops = NULL;
>
> - size = i_size_read(file->f_path.dentry->d_inode);
> - shp->shm_nattch++;
> - shm_unlock(shp);
> -
> down_write(¤t->mm->mmap_sem);
> if (addr && !(shmflg & SHM_REMAP)) {
> - user_addr = ERR_PTR(-EINVAL);
> + err = -EINVAL;
> if (find_vma_intersection(current->mm, addr, addr + size))
> goto invalid;
> /*
> @@ -915,13 +917,17 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg, ulong *raddr)
> goto invalid;
> }
>
> - user_addr = (void*) do_mmap (file, addr, size, prot, flags, 0);
> -
> + user_addr = do_mmap (file, addr, size, prot, flags, 0);
> + *raddr = user_add
Re: [PATCH] shm: Fix the locking and cleanup error handling in do_shmat.
Eric W. Biederman napisał(a):
> When enhancing do_shmat I forgot to take into account that shm_lock
> is a spinlock, and was allocating memory with the lock held.
>
> This patch fixes that by grabbing a reference to the dentry and
> mounts of shm_file before we drop the shm_lock and then performing
> the memory allocations.
>
> This is also a bit of a general scrub on the error handling.
> Everything is now forced through the single return statement
> for clarity, and the handling of the return address now uses
> fewer casts.
>
> Signed-off-by: Eric W. Biederman <[EMAIL PROTECTED]>
> ---
> ipc/shm.c | 56
> 1 files changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index e0b6544..26b935b 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -815,15 +815,16 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg, ulong *raddr)
> unsigned long flags;
> unsigned long prot;
> int acc_mode;
> - void *user_addr;
> + unsigned long user_addr;
> struct ipc_namespace *ns;
> struct shm_file_data *sfd;
> + struct path path;
> mode_t f_mode;
>
> - if (shmid < 0) {
> - err = -EINVAL;
> + err = -EINVAL;
> + if (shmid < 0)
> goto out;
> - } else if ((addr = (ulong)shmaddr)) {
> + else if ((addr = (ulong)shmaddr)) {
> if (addr & (SHMLBA-1)) {
> if (shmflg & SHM_RND)
> addr &= ~(SHMLBA-1); /* round down */
> @@ -831,12 +832,12 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg, ulong *raddr)
> #ifndef __ARCH_FORCE_SHMLBA
> if (addr & ~PAGE_MASK)
> #endif
> - return -EINVAL;
> + goto out;
> }
> flags = MAP_SHARED | MAP_FIXED;
> } else {
> if ((shmflg & SHM_REMAP))
> - return -EINVAL;
> + goto out;
>
> flags = MAP_SHARED;
> }
> @@ -860,7 +861,6 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg, ulong *raddr)
>* additional creator id...
>*/
> ns = current->nsproxy->ipc_ns;
> - err = -EINVAL;
> shp = shm_lock(ns, shmid);
> if(shp == NULL)
> goto out;
> @@ -877,19 +877,25 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg, ulong *raddr)
> if (err)
> goto out_unlock;
>
> + path.dentry = dget(shp->shm_file->f_path.dentry);
> + path.mnt= mntget(shp->shm_file->f_path.mnt);
> + shp->shm_nattch++;
> + size = i_size_read(path.dentry->d_inode);
> + shm_unlock(shp);
> +
> err = -ENOMEM;
> sfd = kzalloc(sizeof(*sfd), GFP_KERNEL);
> if (!sfd)
> - goto out_unlock;
> + goto out_put_path;
>
> + err = -ENOMEM;
> file = get_empty_filp();
> if (!file)
> goto out_free;
>
> file->f_op = &shm_file_operations;
> file->private_data = sfd;
> - file->f_path.dentry = dget(shp->shm_file->f_path.dentry);
> - file->f_path.mnt = mntget(shp->shm_file->f_path.mnt);
> + file->f_path = path;
> file->f_mapping = shp->shm_file->f_mapping;
> file->f_mode = f_mode;
> sfd->id = shp->id;
> @@ -897,13 +903,9 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg, ulong *raddr)
> sfd->file = shp->shm_file;
> sfd->vm_ops = NULL;
>
> - size = i_size_read(file->f_path.dentry->d_inode);
> - shp->shm_nattch++;
> - shm_unlock(shp);
> -
> down_write(¤t->mm->mmap_sem);
> if (addr && !(shmflg & SHM_REMAP)) {
> - user_addr = ERR_PTR(-EINVAL);
> + err = -EINVAL;
> if (find_vma_intersection(current->mm, addr, addr + size))
> goto invalid;
> /*
> @@ -915,13 +917,17 @@ long do_shmat(int shmid, char __user *shmaddr, int
> shmflg, ulong *raddr)
> goto invalid;
> }
>
> - user_addr = (void*) do_mmap (file, addr, size, prot, flags, 0);
> -
> + user_addr = do_mmap (file, addr, size, prot, flags, 0);
> + *raddr = user_addr;
> + err = 0;
> + if (IS_ERR_VALUE(user_addr))
> + err = (long)user_addr;
> invalid:
> up_write(¤t->mm->mmap_sem);
> -
> +
> fput(file);
>
> +out_nattch:
> mutex_lock(&shm_ids(ns).mutex);
> shp = shm_lock(ns, shmid);
> BUG_ON(!shp);
> @@ -933,17 +939,19 @@ invalid:
???
> shm_unlock(shp);
> mutex_unlock(&shm_ids(ns).mutex);
>
> - *raddr = (unsigned long) user_addr;
> - err = 0;
> - if (IS_ERR(user_addr))
> - err = PTR_ERR(user_addr);
> out:
> return err;
> -out_free:
> - kfree(sfd);
> +
> out_unlock:
> shm_unlock(shp);
> goto out;
> +
> +out
