From: Chen Gang <gang.chen@asianux.com>
To: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Michael Kerrisk <mtk.manpages@gmail.com>
Subject: Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
Date: Tue, 24 Sep 2013 11:42:56 +0800 [thread overview]
Message-ID: <52410A40.1000304@asianux.com> (raw)
In-Reply-To: <20130923150625.GB14547@htj.dyndns.org>
On 09/23/2013 11:06 PM, Tejun Heo wrote:
> Hello,
>
> (I have no idea about this but Andrew tagged me, probably thinking it
> was related to cgroups, so here it goes ;)
>
First, thank you for spending your time resource to discuss this patch.
> On Tue, Aug 20, 2013 at 11:01:14AM +0800, Chen Gang wrote:
>> groups_alloc() can return NULL for 'group_info', also group_search()
>> already considers about NULL for 'group_info', so can assume the caller
>> has right to use all related extern functions when 'group_info' is NULL.
>>
>> For groups_free(), need check NULL to match groups_alloc(), just like
>> kmalloc/free().
>
> While it is somewhat customary to make free routines handle NULL
> inputs, the convention isn't a very strong one and given that the
> above change doesn't actually benefit any of the existing use cases,
> it seems gratuituous.
>
Hmm... can user be permitted to call other system call (e.g. getgroups)
before call groups_alloc()? (may the user space already give check, but
for our kernel, we can not only depend on their checking).
According to group_alloc() and setgroups() usage in kernel source code,
'group_info' may be not set if kernel/process is running (although user
space may be sure "if kernel is running, 'group_info' must be set").
The below is the proof for "kernel itself can not be sure 'group_info'
must be set during kernel/process is running", please check, thanks.
The related usage of group_alloc() is below:
arch/s390/kernel/compat_linux.c:257: group_info = groups_alloc(gidsetsize);
drivers/staging/lustre/lustre/include/linux/lustre_common.h:10: ginfo = groups_alloc(0);
drivers/staging/lustre/lustre/ptlrpc/service.c:2287: ginfo = groups_alloc(0);
fs/nfsd/auth.c:46: gi = groups_alloc(0);
fs/nfsd/auth.c:55: gi = groups_alloc(rqgi->ngroups);
kernel/uid16.c:184: group_info = groups_alloc(gidsetsize); /* called by setgroups16 */
kernel/groups.c:241: group_info = groups_alloc(gidsetsize); /* called by setgroups */
net/sunrpc/svcauth_unix.c:506: ug.gi = groups_alloc(gids);
net/sunrpc/svcauth_unix.c:752: cred->cr_group_info = groups_alloc(0);
net/sunrpc/svcauth_unix.c:823: cred->cr_group_info = groups_alloc(slen);
net/sunrpc/auth_gss/gss_rpc_xdr.c:218: creds->cr_group_info = groups_alloc(N);
net/sunrpc/auth_gss/svcauth_gss.c:467: rsci.cred.cr_group_info = groups_alloc(N);
except "kernel/*", others are all related with sub-systems or architectures, so we need search 'setgroups'.
The related usage of setgroups() is below:
arch/ia64/include/uapi/asm/unistd.h:69:#define __NR_setgroups 1078
arch/ia64/kernel/entry.S:1531: data8 sys_setgroups
arch/ia64/kernel/fsys.S:606: data8 0 // setgroups
arch/microblaze/include/uapi/asm/unistd.h:95:#define __NR_setgroups 81 /* ok */
arch/microblaze/include/uapi/asm/unistd.h:222:#define __NR_setgroups32 206 /* ok - without 32 */
arch/microblaze/kernel/syscall_table.S:84: .long sys_setgroups
arch/microblaze/kernel/syscall_table.S:209: .long sys_setgroups
arch/arm64/include/asm/unistd32.h:105:__SYSCALL(81, sys_setgroups16)
arch/arm64/include/asm/unistd32.h:230:__SYSCALL(206, sys_setgroups)
arch/mn10300/include/uapi/asm/unistd.h:95:#define __NR_setgroups 81
arch/mn10300/include/uapi/asm/unistd.h:220:#define __NR_setgroups32 206
arch/mn10300/kernel/entry.S:511: .long sys_setgroups16
arch/mn10300/kernel/entry.S:636: .long sys_setgroups
arch/m68k/include/uapi/asm/unistd.h:89:#define __NR_setgroups 81
arch/m68k/include/uapi/asm/unistd.h:214:#define __NR_setgroups32 206
arch/m68k/kernel/syscalltable.S:104: .long sys_setgroups16
arch/m68k/kernel/syscalltable.S:229: .long sys_setgroups
arch/sparc/include/uapi/asm/unistd.h:120:#define __NR_setgroups 80 /* Common */
arch/sparc/include/uapi/asm/unistd.h:123:#define __NR_setgroups32 82 /* Linux sparc32, setpgrp under SunOS */
arch/sparc/kernel/systbls_64.S:37:/*80*/ .word sys_setgroups16, sys_getpgrp, sys_setgroups, compat_sys_setitimer, sys32_ftruncate64
arch/sparc/kernel/systbls_64.S:115:/*80*/ .word sys_setgroups, sys_getpgrp, sys_nis_syscall, sys_setitimer, sys_nis_syscall
arch/sparc/kernel/systbls_32.S:35:/*80*/ .long sys_setgroups16, sys_getpgrp, sys_setgroups, sys_setitimer, sys_ftruncate64
arch/alpha/include/uapi/asm/unistd.h:84:#define __NR_setgroups 80
arch/alpha/kernel/systbls.S:94: .quad sys_setgroups /* 80 */
arch/sh/include/uapi/asm/unistd_64.h:98:#define __NR_setgroups 81
arch/sh/include/uapi/asm/unistd_64.h:223:#define __NR_setgroups32 206
arch/sh/include/uapi/asm/unistd_32.h:93:#define __NR_setgroups 81
arch/sh/include/uapi/asm/unistd_32.h:218:#define __NR_setgroups32 206
arch/sh/kernel/syscalls_64.S:104: .long sys_setgroups16
arch/sh/kernel/syscalls_64.S:229: .long sys_setgroups
arch/sh/kernel/syscalls_32.S:100: .long sys_setgroups16
arch/sh/kernel/syscalls_32.S:225: .long sys_setgroups
arch/m32r/include/uapi/asm/unistd.h:214:#define __NR_setgroups32 206
arch/m32r/include/asm/unistd.h:39:#define __IGNORE_setgroups
arch/m32r/kernel/syscall_table.S:83: .long sys_ni_syscall /* setgroups16 syscall holder */
arch/m32r/kernel/syscall_table.S:208: .long sys_setgroups
arch/xtensa/include/uapi/asm/unistd.h:431:#define __NR_setgroups 197
arch/xtensa/include/uapi/asm/unistd.h:432:__SYSCALL(197, sys_setgroups, 2)
arch/mips/include/uapi/asm/unistd.h:104:#define __NR_setgroups (__NR_Linux + 81)
arch/mips/include/uapi/asm/unistd.h:503:#define __NR_setgroups (__NR_Linux + 114)
arch/mips/include/uapi/asm/unistd.h:829:#define __NR_setgroups (__NR_Linux + 114)
arch/mips/kernel/scall64-64.S:232: PTR sys_setgroups
arch/mips/kernel/scall64-n32.S:221: PTR sys_setgroups
arch/mips/kernel/scall32-o32.S:317: sys sys_setgroups 2
arch/mips/kernel/scall64-o32.S:276: PTR sys_setgroups
arch/frv/include/uapi/asm/unistd.h:89:#define __NR_setgroups 81
arch/frv/include/uapi/asm/unistd.h:215:#define __NR_setgroups32 206
arch/frv/kernel/entry.S:1261: .long sys_setgroups16
arch/frv/kernel/entry.S:1386: .long sys_setgroups
arch/parisc/hpux/sys_hpux.c:561: "setgroups", /* 80 */
arch/parisc/include/uapi/asm/unistd.h:91:#define __NR_HPUX_setgroups 80
arch/parisc/include/uapi/asm/unistd.h:574:#define __NR_setgroups (__NR_Linux + 81)
arch/parisc/kernel/syscall_table.S:155: ENTRY_SAME(setgroups)
arch/s390/include/uapi/asm/unistd.h:305:#define __NR_setgroups 81
arch/s390/include/uapi/asm/unistd.h:332:#define __NR_setgroups32 206
arch/s390/include/uapi/asm/unistd.h:360:#define __NR_setgroups 206
arch/s390/kernel/compat_linux.c:247:asmlinkage long sys32_setgroups16(int gidsetsize, u16 __user *grouplist)
arch/s390/kernel/compat_wrapper.S:276:ENTRY(sys32_setgroups16_wrapper)
arch/s390/kernel/compat_wrapper.S:279: jg sys32_setgroups16 # branch to system call
arch/s390/kernel/compat_wrapper.S:696:ENTRY(sys32_setgroups_wrapper)
arch/s390/kernel/compat_wrapper.S:699: jg sys_setgroups # branch to system call
arch/s390/kernel/syscalls.S:92:SYSCALL(sys_setgroups16,sys_ni_syscall,sys32_setgroups16_wrapper) /* old setgroups16 syscall */
arch/s390/kernel/syscalls.S:217:SYSCALL(sys_setgroups,sys_setgroups,sys32_setgroups_wrapper)
arch/s390/kernel/compat_linux.h:92:long sys32_setgroups16(int gidsetsize, u16 __user *grouplist);
arch/powerpc/include/uapi/asm/unistd.h:94:#define __NR_setgroups 81
arch/powerpc/include/asm/systbl.h:88:SYSCALL_SPU(setgroups)
arch/arm/include/uapi/asm/unistd.h:109:#define __NR_setgroups (__NR_SYSCALL_BASE+ 81)
arch/arm/include/uapi/asm/unistd.h:234:#define __NR_setgroups32 (__NR_SYSCALL_BASE+206)
arch/arm/kernel/calls.S:93: CALL(sys_setgroups16)
arch/arm/kernel/calls.S:218: CALL(sys_setgroups)
arch/cris/arch-v10/kernel/entry.S:687: .long sys_setgroups16
arch/cris/arch-v10/kernel/entry.S:812: .long sys_setgroups
arch/cris/include/uapi/asm/unistd.h:89:#define __NR_setgroups 81
arch/cris/include/uapi/asm/unistd.h:214:#define __NR_setgroups32 206
arch/cris/arch-v32/kernel/entry.S:633: .long sys_setgroups16
arch/cris/arch-v32/kernel/entry.S:758: .long sys_setgroups
arch/avr32/include/uapi/asm/unistd.h:96:#define __NR_setgroups 81
arch/avr32/kernel/syscall_table.S:97: .long sys_setgroups
arch/x86/syscalls/syscall_32.tbl:90:81 i386 setgroups sys_setgroups16
arch/x86/syscalls/syscall_32.tbl:215:206 i386 setgroups32 sys_setgroups
arch/x86/syscalls/syscall_64.tbl:125:116 common setgroups sys_setgroups
arch/blackfin/include/uapi/asm/unistd.h:93:#define __NR_setgroups 81
arch/blackfin/include/uapi/asm/unistd.h:218:#define __NR_setgroups32 206
arch/blackfin/mach-common/entry.S:1395: .long _sys_setgroups /* setgroups16 */
arch/blackfin/mach-common/entry.S:1520: .long _sys_setgroups
include/uapi/linux/capability.h:134:/* Allows setgroups(2) */
include/uapi/asm-generic/unistd.h:456:#define __NR_setgroups 159
include/uapi/asm-generic/unistd.h:457:__SYSCALL(__NR_setgroups, sys_setgroups)
include/linux/syscalls.h:236:asmlinkage long sys_setgroups(int gidsetsize, gid_t __user *grouplist);
include/linux/syscalls.h:521:asmlinkage long sys_setgroups16(int gidsetsize, old_gid_t __user *grouplist);
kernel/sys_ni.c:132:cond_syscall(sys_setgroups16);
kernel/uid16.c:174:SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
kernel/groups.c:231:SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
all of them are for definitions or declarations.
The related conclusion:
during kernel startup or process creation, kernel does not intend to set 'group_info'.
>> For set_groups(), can allow the caller to set NULL parameter to new
>> 'cred'.
>>
>> For system call getgroups(), if 'cred->group_info' is NULL, need return
>> the related error code (no related data), also need change the related
>> man page ("man 2 getgroups") to complete the return value.
>
> How can cred->group_info be NULL? Can you please describe what issue
> this patch is trying to solve / improve? The patch description
> explains what's being changed but doesn't really offer why such
> changes are being made. Can you please explain why this change is
> beneficial?
>
In extern function groups_search (which also called by export function
in_group_p and in_egroup_p), it checks "if 'cred->group_info' is NULL".
So "kernel/groups.c" have 9 extern/export/system-call functions, and
4/9 notice about "if 'cred->group_info' is NULL" (e.g. groups_alloc,
groups_search, in_group_p, in_egroup_p).
So for API self-consistency, all of extern/export/system-call functions
need notice about it.
> Thanks.
>
Thanks
--
Chen Gang
next prev parent reply other threads:[~2013-09-24 3:44 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-20 3:01 [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions Chen Gang
2013-08-20 3:03 ` Chen Gang
2013-09-03 5:23 ` Chen Gang
2013-09-23 15:06 ` Tejun Heo
2013-09-24 3:42 ` Chen Gang [this message]
2013-09-24 4:06 ` Tejun Heo
2013-09-24 4:27 ` Chen Gang
2013-09-24 12:04 ` Tejun Heo
2013-09-25 0:49 ` Chen Gang
2013-09-25 0:58 ` Tejun Heo
2013-09-25 1:06 ` Chen Gang
2013-09-25 1:14 ` Tejun Heo
2013-09-25 1:47 ` Chen Gang
2013-09-25 4:34 ` Chen Gang
2013-09-26 5:58 ` Chen Gang
2013-09-26 6:30 ` Chen Gang
2013-09-26 13:15 ` Tejun Heo
2013-09-27 1:30 ` Chen Gang
2013-09-27 3:36 ` Tejun Heo
2013-09-27 7:16 ` Chen Gang
2013-09-27 11:53 ` Tejun Heo
2013-09-27 12:19 ` Chen Gang
2013-09-29 2:50 ` Chen Gang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52410A40.1000304@asianux.com \
--to=gang.chen@asianux.com \
--cc=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mtk.manpages@gmail.com \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.