[PATCH V2] audit: log 32-bit socketcalls

[PATCH V2] audit: log 32-bit socketcalls

[Richard Guy Briggs]

32-bit socketcalls were not being logged by audit on x86_64 systems.
Log them.  This is basically a duplicate of the call from
net/socket.c:sys_socketcall(), but it addresses the impedance mismatch
between 32-bit userspace process and 64-bit kernel audit.

See: https://github.com/linux-audit/audit-kernel/issues/14

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

--
v2:
   Move work to audit_socketcall_compat() and use audit_dummy_context().
---
 include/linux/audit.h |   16 ++++++++++++++++
 net/compat.c          |   15 +++++++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 9d4443f..43d8003 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs, unsigned long *args)
 		return __audit_socketcall(nargs, args);
 	return 0;
 }
+static inline int audit_socketcall_compat(int nargs, u32 *args)
+{
+	if (unlikely(!audit_dummy_context())) {
+		int i;
+		unsigned long a[AUDITSC_ARGS];
+
+		for (i=0; i<nargs; i++)
+			a[i] = (unsigned long)args[i];
+		return __audit_socketcall(nargs, a);
+	}
+	return 0;
+}
 static inline int audit_sockaddr(int len, void *addr)
 {
 	if (unlikely(!audit_dummy_context()))
@@ -513,6 +525,10 @@ static inline int audit_socketcall(int nargs, unsigned long *args)
 {
 	return 0;
 }
+static inline int audit_socketcall_compat(int nargs, u32 *args)
+{
+	return 0;
+}
 static inline void audit_fd_pair(int fd1, int fd2)
 { }
 static inline int audit_sockaddr(int len, void *addr)
diff --git a/net/compat.c b/net/compat.c
index 1cd2ec0..f0404d4 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -22,6 +22,7 @@
 #include <linux/filter.h>
 #include <linux/compat.h>
 #include <linux/security.h>
+#include <linux/audit.h>
 #include <linux/export.h>
 
 #include <net/scm.h>
@@ -781,14 +782,24 @@ COMPAT_SYSCALL_DEFINE5(recvmmsg, int, fd, struct compat_mmsghdr __user *, mmsg,
 
 COMPAT_SYSCALL_DEFINE2(socketcall, int, call, u32 __user *, args)
 {
+	unsigned int len;
 	int ret;
-	u32 a[6];
+	u32 a[AUDITSC_ARGS];
 	u32 a0, a1;
 
 	if (call < SYS_SOCKET || call > SYS_SENDMMSG)
 		return -EINVAL;
-	if (copy_from_user(a, args, nas[call]))
+	len = nas[call];
+	if (len > sizeof(a))
+		return -EINVAL;
+
+	if (copy_from_user(a, args, len))
 		return -EFAULT;
+
+	ret = audit_socketcall_compat(len/sizeof(a[0]), a);
+	if (ret)
+		return ret;
+
 	a0 = a[0];
 	a1 = a[1];
 
-- 
1.7.1

Re: [PATCH V2] audit: log 32-bit socketcalls

[Eric Paris]

On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote:
> 32-bit socketcalls were not being logged by audit on x86_64 systems.
> Log them.  This is basically a duplicate of the call from
> net/socket.c:sys_socketcall(), but it addresses the impedance
> mismatch
> between 32-bit userspace process and 64-bit kernel audit.
> 
> See: https://github.com/linux-audit/audit-kernel/issues/14
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> 
> --
> v2:
>    Move work to audit_socketcall_compat() and use
> audit_dummy_context().
> ---
>  include/linux/audit.h |   16 ++++++++++++++++
>  net/compat.c          |   15 +++++++++++++--
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 9d4443f..43d8003 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs,
> unsigned long *args)
>  		return __audit_socketcall(nargs, args);
>  	return 0;
>  }
> +static inline int audit_socketcall_compat(int nargs, u32 *args)
> +{
> +	if (unlikely(!audit_dummy_context())) {

I've always hated these likely/unlikely. Mostly because I think they
are so often wrong. I believe this says that you compiled audit in but
you expect it to be explicitly disabled. While that is (recently) true
in Fedora I highly doubt that's true on the vast majority of systems
that have audit compiled in.

I think all of the likely/unlikely need to just be abandoned, but at
least don't add more? It certainly wouldn't be the first time I was
wrong, and I haven't profiled it. But the function would definitely
look better if coded

static inline int audit_socketcall_compat(int nargs, u32 *args)
{
    if (audit_cummy_context()) {
        return 0
    }
    int i;
    unsigned long a[AUDITSC_ARGS];

    [...]
}

> +		int i;
> +		unsigned long a[AUDITSC_ARGS];
> +
> +		for (i=0; i<nargs; i++)
> +			a[i] = (unsigned long)args[i];
> +		return __audit_socketcall(nargs, a);
> +	}
> +	return 0;
> +}
>  static inline int audit_sockaddr(int len, void *addr)
>  {
>  	if (unlikely(!audit_dummy_context()))
> @@ -513,6 +525,10 @@ static inline int audit_socketcall(int nargs,
> unsigned long *args)
>  {
>  	return 0;
>  }
> +static inline int audit_socketcall_compat(int nargs, u32 *args)
> +{
> +	return 0;
> +}
>  static inline void audit_fd_pair(int fd1, int fd2)
>  { }
>  static inline int audit_sockaddr(int len, void *addr)
> diff --git a/net/compat.c b/net/compat.c
> index 1cd2ec0..f0404d4 100644
> --- a/net/compat.c
> +++ b/net/compat.c
> @@ -22,6 +22,7 @@
>  #include <linux/filter.h>
>  #include <linux/compat.h>
>  #include <linux/security.h>
> +#include <linux/audit.h>
>  #include <linux/export.h>
>  
>  #include <net/scm.h>
> @@ -781,14 +782,24 @@ COMPAT_SYSCALL_DEFINE5(recvmmsg, int, fd,
> struct compat_mmsghdr __user *, mmsg,
>  
>  COMPAT_SYSCALL_DEFINE2(socketcall, int, call, u32 __user *, args)
>  {
> +	unsigned int len;
>  	int ret;
> -	u32 a[6];
> +	u32 a[AUDITSC_ARGS];
>  	u32 a0, a1;
>  
>  	if (call < SYS_SOCKET || call > SYS_SENDMMSG)
>  		return -EINVAL;
> -	if (copy_from_user(a, args, nas[call]))
> +	len = nas[call];
> +	if (len > sizeof(a))
> +		return -EINVAL;
> +
> +	if (copy_from_user(a, args, len))
>  		return -EFAULT;
> +
> +	ret = audit_socketcall_compat(len/sizeof(a[0]), a);
> +	if (ret)
> +		return ret;
> +
>  	a0 = a[0];
>  	a1 = a[1];
>  

Re: [PATCH V2] audit: log 32-bit socketcalls

[Richard Guy Briggs]

On 2017-01-13 09:42, Eric Paris wrote:
> On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote:
> > 32-bit socketcalls were not being logged by audit on x86_64 systems.
> > Log them.  This is basically a duplicate of the call from
> > net/socket.c:sys_socketcall(), but it addresses the impedance
> > mismatch
> > between 32-bit userspace process and 64-bit kernel audit.
> > 
> > See: https://github.com/linux-audit/audit-kernel/issues/14
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > 
> > --
> > v2:
> >    Move work to audit_socketcall_compat() and use
> > audit_dummy_context().
> > ---
> >  include/linux/audit.h |   16 ++++++++++++++++
> >  net/compat.c          |   15 +++++++++++++--
> >  2 files changed, 29 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 9d4443f..43d8003 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs,
> > unsigned long *args)
> >  		return __audit_socketcall(nargs, args);
> >  	return 0;
> >  }
> > +static inline int audit_socketcall_compat(int nargs, u32 *args)
> > +{
> > +	if (unlikely(!audit_dummy_context())) {
> 
> I've always hated these likely/unlikely. Mostly because I think they
> are so often wrong. I believe this says that you compiled audit in but
> you expect it to be explicitly disabled. While that is (recently) true
> in Fedora I highly doubt that's true on the vast majority of systems
> that have audit compiled in.

It has been argued that audit should have pretty much no performance
impact if it is not in use and that if it is, we're willing to take the
more significant overhead of the rest of the code for the sake of one
test to determine whether or not to follow this code path.  The audit
subsystem isn't everyone's favourite baby so not making performance
worse for non-audit-users might help play nice in the community.  I did
contemplate Hanlon's Razor when wondering if this call had been left out
to avoid thunking complications or overhead.

In this case, compat users are less likely to raise a stink about
performance issues, so this situation is not particularly critical.

> I think all of the likely/unlikely need to just be abandoned, but at
> least don't add more? It certainly wouldn't be the first time I was
> wrong, and I haven't profiled it. But the function would definitely
> look better if coded

This is a seperate issue and I agree about simply bailing early rather
than putting the rest of the function in one conditional.  Given the
scoping though, the local variables aren't needed to be allocated in the
unlikely case, but I'm sure the compiler optimizer knows better than we
do how to make this go fast...

In both cases, I thought there was value in making
audit_socketcall_compat() as similar in logic to audit_socketcall() as
possible to make them easier to maintain.

Is either issue a cause for patch respin?  If so, I'll want to normalize
the two functions for consistency.

> static inline int audit_socketcall_compat(int nargs, u32 *args)
> {
>     if (audit_cummy_context()) {
>         return 0
>     }
>     int i;
>     unsigned long a[AUDITSC_ARGS];
> 
>     [...]
> }
> 
> > +		int i;
> > +		unsigned long a[AUDITSC_ARGS];
> > +
> > +		for (i=0; i<nargs; i++)
> > +			a[i] = (unsigned long)args[i];
> > +		return __audit_socketcall(nargs, a);
> > +	}
> > +	return 0;
> > +}
> >  static inline int audit_sockaddr(int len, void *addr)
> >  {
> >  	if (unlikely(!audit_dummy_context()))
> > @@ -513,6 +525,10 @@ static inline int audit_socketcall(int nargs,
> > unsigned long *args)
> >  {
> >  	return 0;
> >  }
> > +static inline int audit_socketcall_compat(int nargs, u32 *args)
> > +{
> > +	return 0;
> > +}
> >  static inline void audit_fd_pair(int fd1, int fd2)
> >  { }
> >  static inline int audit_sockaddr(int len, void *addr)
> > diff --git a/net/compat.c b/net/compat.c
> > index 1cd2ec0..f0404d4 100644
> > --- a/net/compat.c
> > +++ b/net/compat.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/filter.h>
> >  #include <linux/compat.h>
> >  #include <linux/security.h>
> > +#include <linux/audit.h>
> >  #include <linux/export.h>
> >  
> >  #include <net/scm.h>
> > @@ -781,14 +782,24 @@ COMPAT_SYSCALL_DEFINE5(recvmmsg, int, fd,
> > struct compat_mmsghdr __user *, mmsg,
> >  
> >  COMPAT_SYSCALL_DEFINE2(socketcall, int, call, u32 __user *, args)
> >  {
> > +	unsigned int len;
> >  	int ret;
> > -	u32 a[6];
> > +	u32 a[AUDITSC_ARGS];
> >  	u32 a0, a1;
> >  
> >  	if (call < SYS_SOCKET || call > SYS_SENDMMSG)
> >  		return -EINVAL;
> > -	if (copy_from_user(a, args, nas[call]))
> > +	len = nas[call];
> > +	if (len > sizeof(a))
> > +		return -EINVAL;
> > +
> > +	if (copy_from_user(a, args, len))
> >  		return -EFAULT;
> > +
> > +	ret = audit_socketcall_compat(len/sizeof(a[0]), a);
> > +	if (ret)
> > +		return ret;
> > +
> >  	a0 = a[0];
> >  	a1 = a[1];
> >  

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH V2] audit: log 32-bit socketcalls

[Eric Paris]

On Fri, 2017-01-13 at 10:06 -0500, Richard Guy Briggs wrote:
> On 2017-01-13 09:42, Eric Paris wrote:
> > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote:


> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index 9d4443f..43d8003 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -387,6 +387,18 @@ static inline int audit_socketcall(int
> > > nargs,
> > > unsigned long *args)
> > >  		return __audit_socketcall(nargs, args);
> > >  	return 0;
> > >  }
> > > +static inline int audit_socketcall_compat(int nargs, u32 *args)
> > > +{
> > > +	if (unlikely(!audit_dummy_context())) {
> > 
> > I've always hated these likely/unlikely. Mostly because I think
> > they
> > are so often wrong. I believe this says that you compiled audit in
> > but
> > you expect it to be explicitly disabled. While that is (recently)
> > true
> > in Fedora I highly doubt that's true on the vast majority of
> > systems
> > that have audit compiled in.
> 
> It has been argued that audit should have pretty much no performance
> impact if it is not in use and that if it is, we're willing to take
> the
> more significant overhead of the rest of the code for the sake of one
> test to determine whether or not to follow this code path.

Ok, I can buy that argument. Not sure its where I would have settled,
but it does make sense. I'll obviously defer to Paul on what he wants
out of style. I always assume the compiler is brilliant and write
stupid code but your logic is sound there too.

You can/should pretend I said nothing.

Re: [PATCH V2] audit: log 32-bit socketcalls

[Richard Guy Briggs]

On 2017-01-13 10:18, Eric Paris wrote:
> On Fri, 2017-01-13 at 10:06 -0500, Richard Guy Briggs wrote:
> > On 2017-01-13 09:42, Eric Paris wrote:
> > > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote:
> 
> 
> > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > index 9d4443f..43d8003 100644
> > > > --- a/include/linux/audit.h
> > > > +++ b/include/linux/audit.h
> > > > @@ -387,6 +387,18 @@ static inline int audit_socketcall(int
> > > > nargs,
> > > > unsigned long *args)
> > > >  		return __audit_socketcall(nargs, args);
> > > >  	return 0;
> > > >  }
> > > > +static inline int audit_socketcall_compat(int nargs, u32 *args)
> > > > +{
> > > > +	if (unlikely(!audit_dummy_context())) {
> > > 
> > > I've always hated these likely/unlikely. Mostly because I think
> > > they
> > > are so often wrong. I believe this says that you compiled audit in
> > > but
> > > you expect it to be explicitly disabled. While that is (recently)
> > > true
> > > in Fedora I highly doubt that's true on the vast majority of
> > > systems
> > > that have audit compiled in.
> > 
> > It has been argued that audit should have pretty much no performance
> > impact if it is not in use and that if it is, we're willing to take
> > the
> > more significant overhead of the rest of the code for the sake of one
> > test to determine whether or not to follow this code path.
> 
> Ok, I can buy that argument. Not sure its where I would have settled,
> but it does make sense. I'll obviously defer to Paul on what he wants
> out of style. I always assume the compiler is brilliant and write
> stupid code but your logic is sound there too.
> 
> You can/should pretend I said nothing.

You're keeping me honest and making me work for my dinner!  ;-)

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH V2] audit: log 32-bit socketcalls

[Richard Guy Briggs]

On 2017-01-13 10:18, Eric Paris wrote:
> On Fri, 2017-01-13 at 10:06 -0500, Richard Guy Briggs wrote:
> > On 2017-01-13 09:42, Eric Paris wrote:
> > > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote:
> 
> 
> > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > index 9d4443f..43d8003 100644
> > > > --- a/include/linux/audit.h
> > > > +++ b/include/linux/audit.h
> > > > @@ -387,6 +387,18 @@ static inline int audit_socketcall(int
> > > > nargs,
> > > > unsigned long *args)
> > > >  		return __audit_socketcall(nargs, args);
> > > >  	return 0;
> > > >  }
> > > > +static inline int audit_socketcall_compat(int nargs, u32 *args)
> > > > +{
> > > > +	if (unlikely(!audit_dummy_context())) {
> > > 
> > > I've always hated these likely/unlikely. Mostly because I think
> > > they
> > > are so often wrong. I believe this says that you compiled audit in
> > > but
> > > you expect it to be explicitly disabled. While that is (recently)
> > > true
> > > in Fedora I highly doubt that's true on the vast majority of
> > > systems
> > > that have audit compiled in.
> > 
> > It has been argued that audit should have pretty much no performance
> > impact if it is not in use and that if it is, we're willing to take
> > the
> > more significant overhead of the rest of the code for the sake of one
> > test to determine whether or not to follow this code path.
> 
> Ok, I can buy that argument. Not sure its where I would have settled,
> but it does make sense. I'll obviously defer to Paul on what he wants
> out of style. I always assume the compiler is brilliant and write
> stupid code but your logic is sound there too.
> 
> You can/should pretend I said nothing.

You're keeping me honest and making me work for my dinner!  ;-)

- RGB

Re: [PATCH V2] audit: log 32-bit socketcalls

[David Miller]

From: Richard Guy Briggs <rgb@redhat.com>
Date: Fri, 13 Jan 2017 04:51:48 -0500

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 9d4443f..43d8003 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs, unsigned long *args)
>  		return __audit_socketcall(nargs, args);
>  	return 0;
>  }
> +static inline int audit_socketcall_compat(int nargs, u32 *args)
> +{

Please put an empty line between function definitions.

> +	if (unlikely(!audit_dummy_context())) {
> +		int i;
> +		unsigned long a[AUDITSC_ARGS];

Please order local variable declarations from longest to shortest line.

> +
> +		for (i=0; i<nargs; i++)

Please put a space around operators such as "=" and "<".

> +			a[i] = (unsigned long)args[i];
> +		return __audit_socketcall(nargs, a);
> +	}
> +	return 0;
> +}
>  static inline int audit_sockaddr(int len, void *addr)

Again, empty line between function definitions please.

> @@ -781,14 +782,24 @@ COMPAT_SYSCALL_DEFINE5(recvmmsg, int, fd, struct compat_mmsghdr __user *, mmsg,
>  
>  COMPAT_SYSCALL_DEFINE2(socketcall, int, call, u32 __user *, args)
>  {
> +	unsigned int len;
>  	int ret;
> -	u32 a[6];
> +	u32 a[AUDITSC_ARGS];
>  	u32 a0, a1;

Longest to shortest line for local variable declarations please.

Re: [PATCH V2] audit: log 32-bit socketcalls

[Paul Moore]

On Fri, Jan 13, 2017 at 9:42 AM, Eric Paris <eparis@redhat.com> wrote:
> On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote:
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index 9d4443f..43d8003 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs,
>> unsigned long *args)
>>               return __audit_socketcall(nargs, args);
>>       return 0;
>>  }
>> +static inline int audit_socketcall_compat(int nargs, u32 *args)
>> +{
>> +     if (unlikely(!audit_dummy_context())) {
>
> I've always hated these likely/unlikely. Mostly because I think they
> are so often wrong. I believe this says that you compiled audit in but
> you expect it to be explicitly disabled. While that is (recently) true
> in Fedora I highly doubt that's true on the vast majority of systems
> that have audit compiled in.

Richard and I have talked about the likely/unlikely optimization
before and I know Richard likes to use them, but I don't for the
reasons Eric has already mentioned.   Richard, since you're respinning
the patch, go ahead and yank out the unlikely() call.

-- 
paul moore
www.paul-moore.com

Re: [PATCH V2] audit: log 32-bit socketcalls

[Paul Moore]

On Mon, Jan 16, 2017 at 1:27 PM, David Miller <davem@davemloft.net> wrote:
> From: Richard Guy Briggs <rgb@redhat.com>
> Date: Fri, 13 Jan 2017 04:51:48 -0500
>
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index 9d4443f..43d8003 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs, unsigned long *args)
>>               return __audit_socketcall(nargs, args);
>>       return 0;
>>  }
>> +static inline int audit_socketcall_compat(int nargs, u32 *args)
>> +{
>
> Please put an empty line between function definitions.

David, assuming Richard makes your requested changes, any objection if
I merge this via the audit tree instead of the netdev tree?  It's a
bit easier for us from a testing perspective this way ...

>> +     if (unlikely(!audit_dummy_context())) {
>> +             int i;
>> +             unsigned long a[AUDITSC_ARGS];
>
> Please order local variable declarations from longest to shortest line.
>
>> +
>> +             for (i=0; i<nargs; i++)
>
> Please put a space around operators such as "=" and "<".
>
>> +                     a[i] = (unsigned long)args[i];
>> +             return __audit_socketcall(nargs, a);
>> +     }
>> +     return 0;
>> +}
>>  static inline int audit_sockaddr(int len, void *addr)
>
> Again, empty line between function definitions please.
>
>> @@ -781,14 +782,24 @@ COMPAT_SYSCALL_DEFINE5(recvmmsg, int, fd, struct compat_mmsghdr __user *, mmsg,
>>
>>  COMPAT_SYSCALL_DEFINE2(socketcall, int, call, u32 __user *, args)
>>  {
>> +     unsigned int len;
>>       int ret;
>> -     u32 a[6];
>> +     u32 a[AUDITSC_ARGS];
>>       u32 a0, a1;
>
> Longest to shortest line for local variable declarations please.
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit



-- 
paul moore
www.paul-moore.com

Re: [PATCH V2] audit: log 32-bit socketcalls

[David Miller]

From: Paul Moore <paul@paul-moore.com>
Date: Mon, 16 Jan 2017 15:38:33 -0500

> David, assuming Richard makes your requested changes, any objection if
> I merge this via the audit tree instead of the netdev tree?  It's a
> bit easier for us from a testing perspective this way ...

No objection.

Re: [PATCH V2] audit: log 32-bit socketcalls

[Richard Guy Briggs]

On 2017-01-16 15:04, Paul Moore wrote:
> On Fri, Jan 13, 2017 at 9:42 AM, Eric Paris <eparis@redhat.com> wrote:
> > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote:
> >> diff --git a/include/linux/audit.h b/include/linux/audit.h
> >> index 9d4443f..43d8003 100644
> >> --- a/include/linux/audit.h
> >> +++ b/include/linux/audit.h
> >> @@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs,
> >> unsigned long *args)
> >>               return __audit_socketcall(nargs, args);
> >>       return 0;
> >>  }
> >> +static inline int audit_socketcall_compat(int nargs, u32 *args)
> >> +{
> >> +     if (unlikely(!audit_dummy_context())) {
> >
> > I've always hated these likely/unlikely. Mostly because I think they
> > are so often wrong. I believe this says that you compiled audit in but
> > you expect it to be explicitly disabled. While that is (recently) true
> > in Fedora I highly doubt that's true on the vast majority of systems
> > that have audit compiled in.
> 
> Richard and I have talked about the likely/unlikely optimization
> before and I know Richard likes to use them, but I don't for the
> reasons Eric has already mentioned.   Richard, since you're respinning
> the patch, go ahead and yank out the unlikely() call.

I don't "like to use them".  I'm simply following the use and style of
existing code and the arguments of others in places of critical
performance.  If I "fix" that one, then I would feel compelled to yank
out the one in the function immediately above, audit_socketcall() for
consistency to ease my conscience.  Eric conceded that argument.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH V2] audit: log 32-bit socketcalls

[Richard Guy Briggs]

On 2017-01-16 15:04, Paul Moore wrote:
> On Fri, Jan 13, 2017 at 9:42 AM, Eric Paris <eparis@redhat.com> wrote:
> > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote:
> >> diff --git a/include/linux/audit.h b/include/linux/audit.h
> >> index 9d4443f..43d8003 100644
> >> --- a/include/linux/audit.h
> >> +++ b/include/linux/audit.h
> >> @@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs,
> >> unsigned long *args)
> >>               return __audit_socketcall(nargs, args);
> >>       return 0;
> >>  }
> >> +static inline int audit_socketcall_compat(int nargs, u32 *args)
> >> +{
> >> +     if (unlikely(!audit_dummy_context())) {
> >
> > I've always hated these likely/unlikely. Mostly because I think they
> > are so often wrong. I believe this says that you compiled audit in but
> > you expect it to be explicitly disabled. While that is (recently) true
> > in Fedora I highly doubt that's true on the vast majority of systems
> > that have audit compiled in.
> 
> Richard and I have talked about the likely/unlikely optimization
> before and I know Richard likes to use them, but I don't for the
> reasons Eric has already mentioned.   Richard, since you're respinning
> the patch, go ahead and yank out the unlikely() call.

I don't "like to use them".  I'm simply following the use and style of
existing code and the arguments of others in places of critical
performance.  If I "fix" that one, then I would feel compelled to yank
out the one in the function immediately above, audit_socketcall() for
consistency to ease my conscience.  Eric conceded that argument.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH V2] audit: log 32-bit socketcalls

[Richard Guy Briggs]

On 2017-01-16 13:27, David Miller wrote:
> From: Richard Guy Briggs <rgb@redhat.com>
> Date: Fri, 13 Jan 2017 04:51:48 -0500
> 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 9d4443f..43d8003 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs, unsigned long *args)
> >  		return __audit_socketcall(nargs, args);
> >  	return 0;
> >  }
> > +static inline int audit_socketcall_compat(int nargs, u32 *args)
> > +{
> 
> Please put an empty line between function definitions.

Ok, should I reformat the rest of the file while I'm at it?

> > +	if (unlikely(!audit_dummy_context())) {
> > +		int i;
> > +		unsigned long a[AUDITSC_ARGS];
> 
> Please order local variable declarations from longest to shortest line.

Ok.  Is this a recent addition to a style guide or in checkpatch.pl?

> > +
> > +		for (i=0; i<nargs; i++)
> 
> Please put a space around operators such as "=" and "<".

Oops, my bad...

> > +			a[i] = (unsigned long)args[i];
> > +		return __audit_socketcall(nargs, a);
> > +	}
> > +	return 0;
> > +}
> >  static inline int audit_sockaddr(int len, void *addr)
> 
> Again, empty line between function definitions please.
> 
> > @@ -781,14 +782,24 @@ COMPAT_SYSCALL_DEFINE5(recvmmsg, int, fd, struct compat_mmsghdr __user *, mmsg,
> >  
> >  COMPAT_SYSCALL_DEFINE2(socketcall, int, call, u32 __user *, args)
> >  {
> > +	unsigned int len;
> >  	int ret;
> > -	u32 a[6];
> > +	u32 a[AUDITSC_ARGS];
> >  	u32 a0, a1;
> 
> Longest to shortest line for local variable declarations please.

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH V2] audit: log 32-bit socketcalls

[Paul Moore]

On Mon, Jan 16, 2017 at 10:53 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-01-16 15:04, Paul Moore wrote:
>> On Fri, Jan 13, 2017 at 9:42 AM, Eric Paris <eparis@redhat.com> wrote:
>> > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote:
>> >> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> >> index 9d4443f..43d8003 100644
>> >> --- a/include/linux/audit.h
>> >> +++ b/include/linux/audit.h
>> >> @@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs,
>> >> unsigned long *args)
>> >>               return __audit_socketcall(nargs, args);
>> >>       return 0;
>> >>  }
>> >> +static inline int audit_socketcall_compat(int nargs, u32 *args)
>> >> +{
>> >> +     if (unlikely(!audit_dummy_context())) {
>> >
>> > I've always hated these likely/unlikely. Mostly because I think they
>> > are so often wrong. I believe this says that you compiled audit in but
>> > you expect it to be explicitly disabled. While that is (recently) true
>> > in Fedora I highly doubt that's true on the vast majority of systems
>> > that have audit compiled in.
>>
>> Richard and I have talked about the likely/unlikely optimization
>> before and I know Richard likes to use them, but I don't for the
>> reasons Eric has already mentioned.   Richard, since you're respinning
>> the patch, go ahead and yank out the unlikely() call.
>
> I don't "like to use them".  I'm simply following the use and style of
> existing code and the arguments of others in places of critical
> performance.

Okay that's a reasonable reason for why you continued the tradition,
however I'm asking you (for the second time now) not to use it in
audit_socketcall_compat().

> If I "fix" that one, then I would feel compelled to yank
> out the one in the function immediately above, audit_socketcall() for
> consistency to ease my conscience.  Eric conceded that argument.

I'll be very clear on what I want to see in this patch: don't use
likely/unlikely in audit_socketcall_compat() and don't touch it's use
in the other functions at this point in time.

-- 
paul moore
www.paul-moore.com

Re: [PATCH V2] audit: log 32-bit socketcalls

[Paul Moore]

On Mon, Jan 16, 2017 at 10:53 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-01-16 15:04, Paul Moore wrote:
>> On Fri, Jan 13, 2017 at 9:42 AM, Eric Paris <eparis@redhat.com> wrote:
>> > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote:
>> >> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> >> index 9d4443f..43d8003 100644
>> >> --- a/include/linux/audit.h
>> >> +++ b/include/linux/audit.h
>> >> @@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs,
>> >> unsigned long *args)
>> >>               return __audit_socketcall(nargs, args);
>> >>       return 0;
>> >>  }
>> >> +static inline int audit_socketcall_compat(int nargs, u32 *args)
>> >> +{
>> >> +     if (unlikely(!audit_dummy_context())) {
>> >
>> > I've always hated these likely/unlikely. Mostly because I think they
>> > are so often wrong. I believe this says that you compiled audit in but
>> > you expect it to be explicitly disabled. While that is (recently) true
>> > in Fedora I highly doubt that's true on the vast majority of systems
>> > that have audit compiled in.
>>
>> Richard and I have talked about the likely/unlikely optimization
>> before and I know Richard likes to use them, but I don't for the
>> reasons Eric has already mentioned.   Richard, since you're respinning
>> the patch, go ahead and yank out the unlikely() call.
>
> I don't "like to use them".  I'm simply following the use and style of
> existing code and the arguments of others in places of critical
> performance.

Okay that's a reasonable reason for why you continued the tradition,
however I'm asking you (for the second time now) not to use it in
audit_socketcall_compat().

> If I "fix" that one, then I would feel compelled to yank
> out the one in the function immediately above, audit_socketcall() for
> consistency to ease my conscience.  Eric conceded that argument.

I'll be very clear on what I want to see in this patch: don't use
likely/unlikely in audit_socketcall_compat() and don't touch it's use
in the other functions at this point in time.

-- 
paul moore
www.paul-moore.com