linux-kernel - Re: [PATCH v3 1/2] init: add sys-wrapper.h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 31 Aug 2010 23:16:23 +0900
From:	Namhyung Kim <namhyung@...il.com>
To:	Sam Ravnborg <sam@...nborg.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Arnd Bergmann <arnd@...db.de>,
	Phillip Lougher <phillip@...gher.demon.co.uk>,
	Al Viro <viro@...iv.linux.org.uk>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] init: add sys-wrapper.h

On Tue, Aug 31, 2010 at 04:03, Sam Ravnborg <sam@...nborg.org> wrote:
> Hi Namhyung Kim.
>
> Some very basic comments.
>

Hi,
Thanks for your comments.


> On Tue, Aug 31, 2010 at 02:27:49AM +0900, Namhyung Kim wrote:
>> sys-wrapper.h contains wrapper functions for various syscalls used in init
>> code. This wrappers handle proper address space conversion so that it can
>> remove a lot of warnings from sparse.
>>
>> Suggested-by: Arnd Bergmann <arnd@...db.de>
>> Signed-off-by: Namhyung Kim <namhyung@...il.com>
>> ---
>>  init/sys-wrapper.h |  230 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 230 insertions(+), 0 deletions(-)
>>  create mode 100644 init/sys-wrapper.h
>>
>> diff --git a/init/sys-wrapper.h b/init/sys-wrapper.h
>> new file mode 100644
>> index 0000000..9aa904c
>> --- /dev/null
>> +++ b/init/sys-wrapper.h
>> @@ -0,0 +1,230 @@
>> +/*
>> + * init/sys-wrapper.h
> Drop the filename - it has a tendency to get outdated.
>
>> + *
>> + * Copyright (C) 2010  Namhyung Kim <namhyung@...il.com>
>> + *
>> + * wrappers for various syscalls for use in the init code
>> + *
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public
>> + * License v2 as published by the Free Software Foundation.dummy
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public
>> + * License along with this program; if not, write to the
>> + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
>> + * Boston, MA 021110-1307, USA.
>> + */
> Drop the license text. The kernel is covered by GPL v2 anyway.
>
>> +
>> +#include <linux/fs.h>
>> +#include <linux/types.h>
>> +#include <linux/fcntl.h>
>> +#include <linux/dirent.h>
>> +#include <linux/syscalls.h>
>> +
>
> I usually see the inverse christmas tree recommended, that is the longest name first.
>

OK. Then, how about this?
(I added <asm/uaccess.h> and removed <linux/fcntl.h> because I think it is
more appropriate.)

/*
 * wrappers for various syscalls for use in the init code
 *
 * Copyright (C) 2010  Namhyung Kim <namhyung@...il.com>
 *
 * This file is released under the GPLv2.
 */

#include <linux/syscalls.h>
#include <linux/dirent.h>
#include <linux/types.h>
#include <linux/fs.h>

#include <asm/uaccess.h>


>> +
>> +#define kern_sys_call(call, ...)             \
>> +({                                           \
>> +     long result;                            \
>> +     mm_segment_t old_fs = get_fs();         \
>> +     set_fs(KERNEL_DS);                      \
>> +     result = call(__VA_ARGS__);             \
>> +     set_fs(old_fs);                         \
>> +     result;                                 \
>> +})
>> +
>
> Personal preference...
> Replace kern_ with kernel_ all over.
>

Is this just your preference or general tendency?


>> +static inline int kern_sys_mount(char *dev_name, char *dir_name, char *type,
>> +                              unsigned long flags, void *data)
>> +{
>> +     return kern_sys_call(sys_mount,
>> +                          (char __user __force *) dev_name,
>> +                          (char __user __force *) dir_name,
>> +                          (char __user __force *) type,
>> +                          flags,
>> +                          (void __user __force *) data);
>> +}
>> +
>
> I have not tried to investigate the sparse annotations.
> But I wonder whay strings are "(char __user __force *)".
> Is this because the sting usually come from userspace?
>

No, usual strings are in kernel space but syscall requires its arguments
to be __user pointers so we have to convert __force.



-- 
Regards,
Namhyung Kim
--
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/