Re: [PATCH] audit: move audit_get_tty to reduce scope and kabi changes

[Paul Moore]

On Tue, Jun 28, 2016 at 12:07 PM, Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> The only users of audit_get_tty and audit_put_tty are internal to audit,
> so move it out of include/linux/audit.h to kernel.h and create a proper
> function rather than inlining it.  This also reduces kABI changes.
>
> Suggested-by: Paul Moore <pmoore@xxxxxxxxxx>
> Signed-off-by: Richard Guy Briggs <rgb@xxxxxxxxxx>
> ---
>  include/linux/audit.h |   24 ------------------------
>  kernel/audit.c        |   17 +++++++++++++++++
>  kernel/audit.h        |    4 ++++
>  kernel/auditsc.c      |    1 -
>  4 files changed, 21 insertions(+), 25 deletions(-)

With some exceptions, unless it lives under include/uapi it really
isn't a stable API issue for the kernel; all the other kABI fun is a
distribution specific value-add and not something we care about
upstream.  That said, I think this is the better choice.

Further, since I'm already pushing your other patch to Linus for the
next 4.7-rc, I'm going to include this as I think it is worth fixing
now.  Although if Linus balks at this patch I'm not going to fight
very hard for it, I'll just defer it to a future merge window.

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 32cdafb..b40ed5d 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -26,7 +26,6 @@
>  #include <linux/sched.h>
>  #include <linux/ptrace.h>
>  #include <uapi/linux/audit.h>
> -#include <linux/tty.h>
>
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -344,23 +343,6 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
>         return tsk->sessionid;
>  }
>
> -static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> -{
> -       struct tty_struct *tty = NULL;
> -       unsigned long flags;
> -
> -       spin_lock_irqsave(&tsk->sighand->siglock, flags);
> -       if (tsk->signal)
> -               tty = tty_kref_get(tsk->signal->tty);
> -       spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
> -       return tty;
> -}
> -
> -static inline void audit_put_tty(struct tty_struct *tty)
> -{
> -       tty_kref_put(tty);
> -}
> -
>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode);
>  extern void __audit_bprm(struct linux_binprm *bprm);
> @@ -518,12 +500,6 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
>  {
>         return -1;
>  }
> -static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> -{
> -       return NULL;
> -}
> -static inline void audit_put_tty(struct tty_struct *tty)
> -{ }
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  { }
>  static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 384374a..d597101 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1866,6 +1866,23 @@ out_null:
>         audit_log_format(ab, " exe=(null)");
>  }
>
> +struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> +       struct tty_struct *tty = NULL;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&tsk->sighand->siglock, flags);
> +       if (tsk->signal)
> +               tty = tty_kref_get(tsk->signal->tty);
> +       spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
> +       return tty;
> +}
> +
> +void audit_put_tty(struct tty_struct *tty)
> +{
> +       tty_kref_put(tty);
> +}
> +
>  void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
>  {
>         const struct cred *cred;
> diff --git a/kernel/audit.h b/kernel/audit.h
> index cbbe6bb..a492f4c 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -23,6 +23,7 @@
>  #include <linux/audit.h>
>  #include <linux/skbuff.h>
>  #include <uapi/linux/mqueue.h>
> +#include <linux/tty.h>
>
>  /* AUDIT_NAMES is the number of slots we reserve in the audit_context
>   * for saving names from getname().  If we get more names we will allocate
> @@ -262,6 +263,9 @@ extern struct audit_entry *audit_dupe_rule(struct audit_krule *old);
>  extern void audit_log_d_path_exe(struct audit_buffer *ab,
>                                  struct mm_struct *mm);
>
> +extern struct tty_struct *audit_get_tty(struct task_struct *tsk);
> +extern void audit_put_tty(struct tty_struct *tty);
> +
>  /* audit watch functions */
>  #ifdef CONFIG_AUDIT_WATCH
>  extern void audit_put_watch(struct audit_watch *watch);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 33dafa7..60a354e 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -63,7 +63,6 @@
>  #include <asm/unistd.h>
>  #include <linux/security.h>
>  #include <linux/list.h>
> -#include <linux/tty.h>
>  #include <linux/binfmts.h>
>  #include <linux/highmem.h>
>  #include <linux/syscalls.h>
> --
> 1.7.1
>



-- 
paul moore
security @ redhat