Message-ID: <52E898A3.2050401@linaro.org>
Date:	Wed, 29 Jan 2014 14:58:59 +0900
From:	AKASHI Takahiro <takahiro.akashi@...aro.org>
To:	Catalin Marinas <catalin.marinas@....com>
CC:	"viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
	"eparis@...hat.com" <eparis@...hat.com>,
	"rgb@...hat.com" <rgb@...hat.com>,
	Will Deacon <Will.Deacon@....com>,
	"arndb@...db.de" <arndb@...db.de>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linaro-kernel@...ts.linaro.org" <linaro-kernel@...ts.linaro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-audit@...hat.com" <linux-audit@...hat.com>,
	"patches@...aro.org" <patches@...aro.org>
Subject: Re: [PATCH v3] audit: Add generic compat syscall support

Catalin,

Let me correct myself,

On 01/27/2014 09:15 PM, Catalin Marinas wrote:
> On Mon, Jan 27, 2014 at 05:58:07AM +0000, AKASHI Takahiro wrote:
>> Catalin and audit maintainers,
>>
>> On 01/23/2014 11:51 PM, Catalin Marinas wrote:
>>> On Fri, Jan 17, 2014 at 08:03:15AM +0000, AKASHI Takahiro wrote:
>>>> diff --git a/lib/compat_audit.c b/lib/compat_audit.c
>>>> new file mode 100644
>>>> index 0000000..94f6480
>>>> --- /dev/null
>>>> +++ b/lib/compat_audit.c
>>>> @@ -0,0 +1,51 @@
>>>> +#include <linux/init.h>
>>>> +#include <linux/types.h>
>>>> +/* FIXME: this might be architecture dependent */
>>>> +#include <asm/unistd_32.h>
>>>
>>> It most likely is architecture dependent.
>>
>> I'm wondering what name is the most appropriate in this case.
>> Most archictures have __NR_xyz definitions in "unistd_32.h",
>> but arm64 doesn't have it, instead "unistd32." which contains
>> only __SYSCALL(xyz, NO). Confusing?
>
> I don't think we should introduce a new file (or at least it should be
> named something containing "audit" to make it clearer).
>
>>>> +int audit_classify_compat_syscall(int abi, unsigned syscall)
>>>> +{
>>>> +	switch (syscall) {
>>>> +#ifdef __NR_open
>>>> +	case __NR_open:
>>>> +		return 2;
>>>> +#endif
>>>> +#ifdef __NR_openat
>>>> +	case __NR_openat:
>>>> +		return 3;
>>>> +#endif
>>>> +#ifdef __NR_socketcall
>>>> +	case __NR_socketcall:
>>>> +		return 4;
>>>> +#endif
>>>> +	case __NR_execve:
>>>> +		return 5;
>>>> +	default:
>>>> +		return 1;
>>>> +	}
>>>> +}
>>>
>>> BTW, since they aren't many, you could get the arch code to define
>>> __NR_compat_open etc. explicitly and use these. On arm64 we have a few
>>> of these defined to avoid name collision in signal handling code.
>>
>> Again, most architecture have their own unistd32.h for compat system calls,
>> and use __NR_open-like naming.
>> It's unlikely for these archs to migrate to "generic compat" auditing,
>> but I believe that '__NR_open'-like naming is better because we may be able to avoid
>> arch-specific changes even for future(?) syscall-related enhancements in audit.

In my compat_audit.c, all the entries in audit classes are derived from asm-generic/audit_*.h,
where __NR_xyz are used to list the system calls. So it is not possible to use __NR_compat_xyz
as far as we re-use those generic files.
(Obviously we don't want to duplicate those header files, that is, audit_compat_*.h.)

I agree that we should not have similar but different files, like unist32.h and unistd_32.h,
but it seems to be inevitable for our case. (That is the reason why I dynamically generate unistd_32.h)

As for arch-specific header file name, "audit_unistd32.h" can be fine, but people on other architectures
might be unhappy with such a name since they can commonly use unistd32.h instead.


- Takahiro AKASHI

> My preference is as above, a few __NR_compat_* (just those required by
> audit) defined in unistd.h but I'm not an audit maintainer.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists