On 4 September 2014 15:38, Jeff Layton <[email protected]> wrote:
> security_file_set_fowner always returns 0, so make it f_setown and
> __f_setown void return functions and fix up the error handling in the
> callers.
>
> Cc: [email protected]
> Cc: Christoph Hellwig <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> drivers/net/tun.c | 4 +---
> drivers/tty/tty_io.c | 3 ++-
> fs/fcntl.c | 21 +++++++--------------
> fs/locks.c | 2 +-
> fs/notify/dnotify/dnotify.c | 8 +-------
> include/linux/fs.h | 4 ++--
> include/linux/security.h | 8 ++++----
> net/socket.c | 3 ++-
> security/capability.c | 4 ++--
> security/security.c | 2 +-
> security/selinux/hooks.c | 4 +---
> security/smack/smack_lsm.c | 3 +--
> 12 files changed, 25 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index acaaf6784179..186ce541c657 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2152,9 +2152,7 @@ static int tun_chr_fasync(int fd, struct file *file, int on)
> goto out;
>
> if (on) {
> - ret = __f_setown(file, task_pid(current), PIDTYPE_PID, 0);
> - if (ret)
> - goto out;
> + __f_setown(file, task_pid(current), PIDTYPE_PID, 0);
> tfile->flags |= TUN_FASYNC;
> } else
> tfile->flags &= ~TUN_FASYNC;
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 8fbad3410c75..aea3b66f7bf2 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2163,8 +2163,9 @@ static int __tty_fasync(int fd, struct file *filp, int on)
> }
> get_pid(pid);
> spin_unlock_irqrestore(&tty->ctrl_lock, flags);
> - retval = __f_setown(filp, pid, type, 0);
> + __f_setown(filp, pid, type, 0);
> put_pid(pid);
> + retval = 0;
> }
> out:
> return retval;
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 22d1c3df61ac..99d440a4a6ba 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -98,26 +98,19 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
> write_unlock_irq(&filp->f_owner.lock);
> }
>
> -int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
> +void __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
> int force)
> {
> - int err;
> -
> - err = security_file_set_fowner(filp);
> - if (err)
> - return err;
> -
> + security_file_set_fowner(filp);
> f_modown(filp, pid, type, force);
> - return 0;
> }
> EXPORT_SYMBOL(__f_setown);
>
> -int f_setown(struct file *filp, unsigned long arg, int force)
> +void f_setown(struct file *filp, unsigned long arg, int force)
> {
> enum pid_type type;
> struct pid *pid;
> int who = arg;
> - int result;
> type = PIDTYPE_PID;
> if (who < 0) {
> type = PIDTYPE_PGID;
> @@ -125,9 +118,8 @@ int f_setown(struct file *filp, unsigned long arg, int force)
> }
> rcu_read_lock();
> pid = find_vpid(who);
> - result = __f_setown(filp, pid, type, force);
> + __f_setown(filp, pid, type, force);
> rcu_read_unlock();
> - return result;
> }
> EXPORT_SYMBOL(f_setown);
>
> @@ -181,7 +173,7 @@ static int f_setown_ex(struct file *filp, unsigned long arg)
> if (owner.pid && !pid)
> ret = -ESRCH;
> else
> - ret = __f_setown(filp, pid, type, 1);
> + __f_setown(filp, pid, type, 1);
> rcu_read_unlock();
>
> return ret;
> @@ -302,7 +294,8 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> force_successful_syscall_return();
> break;
> case F_SETOWN:
> - err = f_setown(filp, arg, 1);
> + f_setown(filp, arg, 1);
> + err = 0;
> break;
> case F_GETOWN_EX:
> err = f_getown_ex(filp, arg);
> diff --git a/fs/locks.c b/fs/locks.c
> index d7e15a256f8f..18e87f11a25f 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1776,7 +1776,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
> if (!fasync_insert_entry(fd, filp, &ret->fl_fasync, new))
> new = NULL;
>
> - error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
> + __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
> out_unlock:
> spin_unlock(&inode->i_lock);
> if (fl)
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index abc8cbcfe90e..caaaf9dfe353 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -346,13 +346,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out;
> }
>
> - error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
> - if (error) {
> - /* if we added, we must shoot */
> - if (dn_mark == new_dn_mark)
> - destroy = 1;
> - goto out;
> - }
> + __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
>
> error = attach_dn(dn, dn_mark, id, fd, filp, mask);
> /* !error means that we attached the dn to the dn_mark, so don't free it */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 435e3d9ec5cf..96528f73dda4 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1139,8 +1139,8 @@ extern void fasync_free(struct fasync_struct *);
> /* can be called from interrupts */
> extern void kill_fasync(struct fasync_struct **, int, int);
>
> -extern int __f_setown(struct file *filp, struct pid *, enum pid_type, int force);
> -extern int f_setown(struct file *filp, unsigned long arg, int force);
> +extern void __f_setown(struct file *filp, struct pid *, enum pid_type, int force);
> +extern void f_setown(struct file *filp, unsigned long arg, int force);
> extern void f_delown(struct file *filp);
> extern pid_t f_getown(struct file *filp);
> extern int send_sigurg(struct fown_struct *fown);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 623f90e5f38d..b10e7af95d3b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1559,7 +1559,7 @@ struct security_operations {
> int (*file_lock) (struct file *file, unsigned int cmd);
> int (*file_fcntl) (struct file *file, unsigned int cmd,
> unsigned long arg);
> - int (*file_set_fowner) (struct file *file);
> + void (*file_set_fowner) (struct file *file);
> int (*file_send_sigiotask) (struct task_struct *tsk,
> struct fown_struct *fown, int sig);
> int (*file_receive) (struct file *file);
> @@ -1834,7 +1834,7 @@ int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
> unsigned long prot);
> int security_file_lock(struct file *file, unsigned int cmd);
> int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
> -int security_file_set_fowner(struct file *file);
> +void security_file_set_fowner(struct file *file);
> int security_file_send_sigiotask(struct task_struct *tsk,
> struct fown_struct *fown, int sig);
> int security_file_receive(struct file *file);
> @@ -2312,9 +2312,9 @@ static inline int security_file_fcntl(struct file *file, unsigned int cmd,
> return 0;
> }
>
> -static inline int security_file_set_fowner(struct file *file)
> +static inline void security_file_set_fowner(struct file *file)
> {
> - return 0;
> + return;
> }
>
> static inline int security_file_send_sigiotask(struct task_struct *tsk,
> diff --git a/net/socket.c b/net/socket.c
> index 95ee7d8682e7..769c9671847e 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1069,7 +1069,8 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
> err = -EFAULT;
> if (get_user(pid, (int __user *)argp))
> break;
> - err = f_setown(sock->file, pid, 1);
> + f_setown(sock->file, pid, 1);
> + err = 0;
> break;
> case FIOGETOWN:
> case SIOCGPGRP:
> diff --git a/security/capability.c b/security/capability.c
> index a74fde6a7468..d68c57a62bcf 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -343,9 +343,9 @@ static int cap_file_fcntl(struct file *file, unsigned int cmd,
> return 0;
> }
>
> -static int cap_file_set_fowner(struct file *file)
> +static void cap_file_set_fowner(struct file *file)
> {
> - return 0;
> + return;
> }
>
> static int cap_file_send_sigiotask(struct task_struct *tsk,
> diff --git a/security/security.c b/security/security.c
> index e41b1a8d7644..fd301c177d04 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -775,7 +775,7 @@ int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
> return security_ops->file_fcntl(file, cmd, arg);
> }
>
> -int security_file_set_fowner(struct file *file)
> +void security_file_set_fowner(struct file *file)
> {
> return security_ops->file_set_fowner(file);
If file_set_fowner op is now type of "void", how you can actually
return the value?
I think compiler must give error. How could you compile it?
- Dmitry
> }
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index b0e940497e23..ada0d0bf3463 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3346,14 +3346,12 @@ static int selinux_file_fcntl(struct file *file, unsigned int cmd,
> return err;
> }
>
> -static int selinux_file_set_fowner(struct file *file)
> +static void selinux_file_set_fowner(struct file *file)
> {
> struct file_security_struct *fsec;
>
> fsec = file->f_security;
> fsec->fown_sid = current_sid();
> -
> - return 0;
> }
>
> static int selinux_file_send_sigiotask(struct task_struct *tsk,
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index e6ab307ce86e..69e5635d89e5 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1390,12 +1390,11 @@ static int smack_mmap_file(struct file *file,
> * Returns 0
> * Further research may be required on this one.
> */
> -static int smack_file_set_fowner(struct file *file)
> +static void smack_file_set_fowner(struct file *file)
> {
> struct smack_known *skp = smk_of_current();
>
> file->f_security = skp->smk_known;
> - return 0;
> }
>
> /**
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
On Tue, Oct 07, 2014 at 08:11:42PM +0300, Dmitry Kasatkin wrote:
> If file_set_fowner op is now type of "void", how you can actually
> return the value?
> I think compiler must give error. How could you compile it?
Returning void values is a GNU extension. I've seen it in a few
places in the kernel. Although in general I'd prefer it we'd remove
it (and have the compiler warn about it).
On 7 October 2014 20:17, Christoph Hellwig <[email protected]> wrote:
> On Tue, Oct 07, 2014 at 08:11:42PM +0300, Dmitry Kasatkin wrote:
>> If file_set_fowner op is now type of "void", how you can actually
>> return the value?
>> I think compiler must give error. How could you compile it?
>
> Returning void values is a GNU extension. I've seen it in a few
> places in the kernel. Although in general I'd prefer it we'd remove
> it (and have the compiler warn about it).
>
Cool. There were patches around to use LLVM as well.
Will it work there?
--
Thanks,
Dmitry
On Tue, 7 Oct 2014 20:34:57 +0300
Dmitry Kasatkin <[email protected]> wrote:
> On 7 October 2014 20:17, Christoph Hellwig <[email protected]> wrote:
> > On Tue, Oct 07, 2014 at 08:11:42PM +0300, Dmitry Kasatkin wrote:
> >> If file_set_fowner op is now type of "void", how you can actually
> >> return the value?
> >> I think compiler must give error. How could you compile it?
> >
> > Returning void values is a GNU extension. I've seen it in a few
> > places in the kernel. Although in general I'd prefer it we'd remove
> > it (and have the compiler warn about it).
> >
>
> Cool. There were patches around to use LLVM as well.
> Will it work there?
>
Well, no one has complained so far, and this has been in linux-next for
more than a month or so. I was just getting ready to send my pull
request to Linus, so you caught this just in time.
I'll respin this patch to not call "return" there and let it stew in
-next for a couple of days before I send the pull request.
Thanks,
--
Jeff Layton <[email protected]>