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]

Message-ID: <20100830190330.GB12921@merkur.ravnborg.org>
Date:	Mon, 30 Aug 2010 21:03:30 +0200
From:	Sam Ravnborg <sam@...nborg.org>
To:	Namhyung Kim <namhyung@...il.com>
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

Hi Namhyung Kim.

Some very basic 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.
> + *
> + * 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.


> +
> +#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.

> +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?


	Sam
--
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/