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().
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.
Signed-off-by: Chen Gang <[email protected]>
---
kernel/groups.c | 14 +++++++++++---
1 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/kernel/groups.c b/kernel/groups.c
index 6b2588d..a21a4ce 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -52,6 +52,9 @@ EXPORT_SYMBOL(groups_alloc);
void groups_free(struct group_info *group_info)
{
+ if (!group_info)
+ return;
+
if (group_info->blocks[0] != group_info->small_block) {
int i;
for (i = 0; i < group_info->nblocks; i++)
@@ -163,9 +166,12 @@ int groups_search(const struct group_info
*group_info, kgid_t grp)
*/
int set_groups(struct cred *new, struct group_info *group_info)
{
- put_group_info(new->group_info);
- groups_sort(group_info);
- get_group_info(group_info);
+ if (new->group_info)
+ put_group_info(new->group_info);
+ if (group_info) {
+ groups_sort(group_info);
+ get_group_info(group_info);
+ }
new->group_info = group_info;
return 0;
}
@@ -206,6 +212,8 @@ SYSCALL_DEFINE2(getgroups, int, gidsetsize, gid_t
__user *, grouplist)
if (gidsetsize < 0)
return -EINVAL;
+ if (!cred->group_info)
+ return -ENODATA;
/* no need to grab task_lock here; it cannot change */
i = cred->group_info->ngroups;
--
1.7.7.6
If this patch is correct, also need modify the man page for the return
value of getgroups().
Thanks.
On 08/20/2013 11:01 AM, 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().
>
> 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.
>
>
> Signed-off-by: Chen Gang <[email protected]>
> ---
> kernel/groups.c | 14 +++++++++++---
> 1 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/groups.c b/kernel/groups.c
> index 6b2588d..a21a4ce 100644
> --- a/kernel/groups.c
> +++ b/kernel/groups.c
> @@ -52,6 +52,9 @@ EXPORT_SYMBOL(groups_alloc);
>
> void groups_free(struct group_info *group_info)
> {
> + if (!group_info)
> + return;
> +
> if (group_info->blocks[0] != group_info->small_block) {
> int i;
> for (i = 0; i < group_info->nblocks; i++)
> @@ -163,9 +166,12 @@ int groups_search(const struct group_info
> *group_info, kgid_t grp)
> */
> int set_groups(struct cred *new, struct group_info *group_info)
> {
> - put_group_info(new->group_info);
> - groups_sort(group_info);
> - get_group_info(group_info);
> + if (new->group_info)
> + put_group_info(new->group_info);
> + if (group_info) {
> + groups_sort(group_info);
> + get_group_info(group_info);
> + }
> new->group_info = group_info;
> return 0;
> }
> @@ -206,6 +212,8 @@ SYSCALL_DEFINE2(getgroups, int, gidsetsize, gid_t
> __user *, grouplist)
>
> if (gidsetsize < 0)
> return -EINVAL;
> + if (!cred->group_info)
> + return -ENODATA;
>
> /* no need to grab task_lock here; it cannot change */
> i = cred->group_info->ngroups;
>
--
Chen Gang
Hello Maintainers:
Please help check this patch, when you have time.
If need a related test, please let me know, I should try (better to
provide some suggestions for test).
Thanks.
On 08/20/2013 11:03 AM, Chen Gang wrote:
>
> If this patch is correct, also need modify the man page for the return
> value of getgroups().
>
> Thanks.
>
> On 08/20/2013 11:01 AM, 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().
>>
>> 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.
>>
>>
>> Signed-off-by: Chen Gang <[email protected]>
>> ---
>> kernel/groups.c | 14 +++++++++++---
>> 1 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/groups.c b/kernel/groups.c
>> index 6b2588d..a21a4ce 100644
>> --- a/kernel/groups.c
>> +++ b/kernel/groups.c
>> @@ -52,6 +52,9 @@ EXPORT_SYMBOL(groups_alloc);
>>
>> void groups_free(struct group_info *group_info)
>> {
>> + if (!group_info)
>> + return;
>> +
>> if (group_info->blocks[0] != group_info->small_block) {
>> int i;
>> for (i = 0; i < group_info->nblocks; i++)
>> @@ -163,9 +166,12 @@ int groups_search(const struct group_info
>> *group_info, kgid_t grp)
>> */
>> int set_groups(struct cred *new, struct group_info *group_info)
>> {
>> - put_group_info(new->group_info);
>> - groups_sort(group_info);
>> - get_group_info(group_info);
>> + if (new->group_info)
>> + put_group_info(new->group_info);
>> + if (group_info) {
>> + groups_sort(group_info);
>> + get_group_info(group_info);
>> + }
>> new->group_info = group_info;
>> return 0;
>> }
>> @@ -206,6 +212,8 @@ SYSCALL_DEFINE2(getgroups, int, gidsetsize, gid_t
>> __user *, grouplist)
>>
>> if (gidsetsize < 0)
>> return -EINVAL;
>> + if (!cred->group_info)
>> + return -ENODATA;
>>
>> /* no need to grab task_lock here; it cannot change */
>> i = cred->group_info->ngroups;
>>
>
>
--
Chen Gang
Hello,
(I have no idea about this but Andrew tagged me, probably thinking it
was related to cgroups, so here it goes ;)
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.
> 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?
Thanks.
--
tejun
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
Hello,
On Tue, Sep 24, 2013 at 11:42:56AM +0800, Chen Gang wrote:
> 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).
I don't think so.
> 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 conclusion:
>
> during kernel startup or process creation, kernel does not intend to set 'group_info'.
No, this is not a proof or any meaningful conclusion. This is just
some random suspicions combined with supposedly related grep output.
> 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.
I'm afraid this isn't useful. If you want to change the code, you
actually need to understand what's going on. "this seems weird to me"
is a good starting point but you need to go way beyond that to
actually make changes.
Thanks.
--
tejun
On 09/24/2013 12:06 PM, Tejun Heo wrote:
> Hello,
>
> On Tue, Sep 24, 2013 at 11:42:56AM +0800, Chen Gang wrote:
>> 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).
>
> I don't think so.
>
>> 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 conclusion:
>>
>> during kernel startup or process creation, kernel does not intend to set 'group_info'.
>
> No, this is not a proof or any meaningful conclusion. This is just
> some random suspicions combined with supposedly related grep output.
>
For real world, /sbin/init will call setgroups, so user space 'help'
kernel itself to protect this issue, but I think, we don't only depend
on the user space help checking.
The proof is below:
[root@gchenlinux tmp]# grep setgroups /sbin/*
Binary file /sbin/init matches
Binary file /sbin/rpc.statd matches
Binary file /sbin/rsyslogd matches
Binary file /sbin/runuser matches
>From reading kernel source code, kernel itself does not intend to set
'group_info', it is triggered by user space or another kernel mode
sub-systems.
>> 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.
>
> I'm afraid this isn't useful. If you want to change the code, you
> actually need to understand what's going on. "this seems weird to me"
> is a good starting point but you need to go way beyond that to
> actually make changes.
>
If some of APIs check the related parameters, but the other not, this
interface must assume some usage condition from the callers which
callers can break out (although the callers may not break out, now).
> Thanks.
>
--
Chen Gang
On Tue, Sep 24, 2013 at 12:27:36PM +0800, Chen Gang wrote:
> For real world, /sbin/init will call setgroups, so user space 'help'
> kernel itself to protect this issue, but I think, we don't only depend
> on the user space help checking.
>
> The proof is below:
>
> [root@gchenlinux tmp]# grep setgroups /sbin/*
> Binary file /sbin/init matches
> Binary file /sbin/rpc.statd matches
> Binary file /sbin/rsyslogd matches
> Binary file /sbin/runuser matches
>
> From reading kernel source code, kernel itself does not intend to set
> 'group_info', it is triggered by user space or another kernel mode
> sub-systems.
Can you please demonstrate such failure? You can tell kernel to
execute a given binary instead of init with "init=" param.
Thanks.
--
tejun
On 09/24/2013 08:04 PM, Tejun Heo wrote:
> On Tue, Sep 24, 2013 at 12:27:36PM +0800, Chen Gang wrote:
>> For real world, /sbin/init will call setgroups, so user space 'help'
>> kernel itself to protect this issue, but I think, we don't only depend
>> on the user space help checking.
>>
>> The proof is below:
>>
>> [root@gchenlinux tmp]# grep setgroups /sbin/*
>> Binary file /sbin/init matches
>> Binary file /sbin/rpc.statd matches
>> Binary file /sbin/rsyslogd matches
>> Binary file /sbin/runuser matches
>>
>> From reading kernel source code, kernel itself does not intend to set
>> 'group_info', it is triggered by user space or another kernel mode
>> sub-systems.
>
> Can you please demonstrate such failure? You can tell kernel to
> execute a given binary instead of init with "init=" param.
>
OK, I will/should try, today (although I have to spend my additional
time resource on it. :-( ).
Hmm... in fact, in my opinion, interface (especially content system
call) need make itself consistency, although at present, it can not
cause issue.
> Thanks.
>
Thanks.
--
Chen Gang
Hello, Chen.
On Wed, Sep 25, 2013 at 08:49:31AM +0800, Chen Gang wrote:
> > Can you please demonstrate such failure? You can tell kernel to
> > execute a given binary instead of init with "init=" param.
>
> OK, I will/should try, today (although I have to spend my additional
> time resource on it. :-( ).
Please note that that's not necessarily "additional" resource that you
spend. It's more of something necessary to justify the changes you're
suggesting. It's true that not all bug fixes / improvements require
explicit demonstration but I'm not very convinced about your analysis
partly because I'm not too familiar with the code path but also
because the code has been stable with years and you seem pretty new to
the area.
> Hmm... in fact, in my opinion, interface (especially content system
> call) need make itself consistency, although at present, it can not
> cause issue.
Sure, it should be consistent but I'm not sure what you're perceiving
as inconsistent is actually inconsistent. Anyways, let's please have
something deomnstratable. We can think more about the interface
niggles afterwards.
Thanks.
--
tejun
On 09/25/2013 08:58 AM, Tejun Heo wrote:
> Hello, Chen.
>
> On Wed, Sep 25, 2013 at 08:49:31AM +0800, Chen Gang wrote:
>>> Can you please demonstrate such failure? You can tell kernel to
>>> execute a given binary instead of init with "init=" param.
>>
>> OK, I will/should try, today (although I have to spend my additional
>> time resource on it. :-( ).
>
> Please note that that's not necessarily "additional" resource that you
> spend. It's more of something necessary to justify the changes you're
> suggesting. It's true that not all bug fixes / improvements require
> explicit demonstration but I'm not very convinced about your analysis
> partly because I'm not too familiar with the code path but also
> because the code has been stable with years and you seem pretty new to
> the area.
>
>> Hmm... in fact, in my opinion, interface (especially content system
>> call) need make itself consistency, although at present, it can not
>> cause issue.
>
> Sure, it should be consistent but I'm not sure what you're perceiving
> as inconsistent is actually inconsistent. Anyways, let's please have
> something deomnstratable. We can think more about the interface
> niggles afterwards.
>
> Thanks.
>
OK, I see, the 'root cause' is: "you are not the related maintainer
either", so it is really necessary for me to spend additional time
resource on it :-(.
But all together, thank you for spending your time resource on it, let
us continue :-).
Thanks.
--
Chen Gang
On Wed, Sep 25, 2013 at 09:06:52AM +0800, Chen Gang wrote:
> OK, I see, the 'root cause' is: "you are not the related maintainer
> either", so it is really necessary for me to spend additional time
> resource on it :-(.
Yeah, at least partly. That and the fact that I'm not too willing to
dig into the code without further evidence. It isn't anything strange
to ask tho and I'm likely to do that even for subsystems that I know
intimately if the subject code has been stable / stale for years and
the analysis doesn't seem immediately convincing. And, if my
experience is anything to go by, it's not too unlikely that you might
hit something which doesn't agree with your current assumptions while
trying to actually trigger the problem.
Thanks.
--
tejun
On 09/25/2013 09:14 AM, Tejun Heo wrote:
> On Wed, Sep 25, 2013 at 09:06:52AM +0800, Chen Gang wrote:
>> OK, I see, the 'root cause' is: "you are not the related maintainer
>> either", so it is really necessary for me to spend additional time
>> resource on it :-(.
>
> Yeah, at least partly. That and the fact that I'm not too willing to
> dig into the code without further evidence. It isn't anything strange
> to ask tho and I'm likely to do that even for subsystems that I know
> intimately if the subject code has been stable / stale for years and
> the analysis doesn't seem immediately convincing. And, if my
> experience is anything to go by, it's not too unlikely that you might
> hit something which doesn't agree with your current assumptions while
> trying to actually trigger the problem.
>
> Thanks.
>
Excuse me, my English is not quite well, I do not quite understand what
you said (but it seems what you said is reasonable, and not need reply).
Thanks.
--
Chen Gang
On 09/25/2013 09:47 AM, Chen Gang wrote:
> On 09/25/2013 09:14 AM, Tejun Heo wrote:
>> On Wed, Sep 25, 2013 at 09:06:52AM +0800, Chen Gang wrote:
>>> OK, I see, the 'root cause' is: "you are not the related maintainer
>>> either", so it is really necessary for me to spend additional time
>>> resource on it :-(.
>>
>> Yeah, at least partly. That and the fact that I'm not too willing to
>> dig into the code without further evidence. It isn't anything strange
>> to ask tho and I'm likely to do that even for subsystems that I know
>> intimately if the subject code has been stable / stale for years and
>> the analysis doesn't seem immediately convincing. And, if my
>> experience is anything to go by, it's not too unlikely that you might
>> hit something which doesn't agree with your current assumptions while
>> trying to actually trigger the problem.
>>
>> Thanks.
>>
>
Excuse me, I have to do some urgent internal things within my company,
so the test and confirmation may be delayed (I will try to finish test
within this month: 2013-09-30).
> Excuse me, my English is not quite well, I do not quite understand what
> you said (but it seems what you said is reasonable, and not need reply).
>
>
> Thanks.
>
--
Chen Gang
On 09/25/2013 12:34 PM, Chen Gang wrote:
> On 09/25/2013 09:47 AM, Chen Gang wrote:
>> On 09/25/2013 09:14 AM, Tejun Heo wrote:
>>> On Wed, Sep 25, 2013 at 09:06:52AM +0800, Chen Gang wrote:
>>>> OK, I see, the 'root cause' is: "you are not the related maintainer
>>>> either", so it is really necessary for me to spend additional time
>>>> resource on it :-(.
>>>
>>> Yeah, at least partly. That and the fact that I'm not too willing to
>>> dig into the code without further evidence. It isn't anything strange
>>> to ask tho and I'm likely to do that even for subsystems that I know
>>> intimately if the subject code has been stable / stale for years and
>>> the analysis doesn't seem immediately convincing. And, if my
>>> experience is anything to go by, it's not too unlikely that you might
>>> hit something which doesn't agree with your current assumptions while
>>> trying to actually trigger the problem.
>>>
>>> Thanks.
>>>
Oh, not cause issue, the reason is "'groups' exports extern variable
'init_groups', when start process, default 'cred' will set it to be
sure of groups always be initialized".
Hmm... but after all, I still think this file need be improved: "remove
the group_info checking in groups_search()", please help check, thanks.
-------------------------------patch begin------------------------------
kernel/groups.c: remove useless "!group_info" checking in groups_search().
Since groups_free() need not check 'group_info', groups_search() need
not, either, and then in_group_p() and in_egroup_p(), either.
'groups' assumes kernel mode callers are sure of 'group_info' valid.
When process starts, the related kernel mode caller need set default
'group_info' firstly (extern variable 'init_group').
If groups_alloc() fails, the caller must not call any related API again
with the related invalid 'group_info'.
Signed-off-by: Chen Gang <[email protected]>
---
kernel/groups.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/kernel/groups.c b/kernel/groups.c
index 90cf1c3..0a7f81d 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -136,9 +136,6 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
{
unsigned int left, right;
- if (!group_info)
- return 0;
-
left = 0;
right = group_info->ngroups;
while (left < right) {
--
1.7.7.6
-------------------------------patch end--------------------------------
>>
>
> Excuse me, I have to do some urgent internal things within my company,
> so the test and confirmation may be delayed (I will try to finish test
> within this month: 2013-09-30).
>
>> Excuse me, my English is not quite well, I do not quite understand what
>> you said (but it seems what you said is reasonable, and not need reply).
>>
>>
>> Thanks.
>>
>
>
Thanks.
--
Chen Gang
On 09/26/2013 01:58 PM, Chen Gang wrote:
> On 09/25/2013 12:34 PM, Chen Gang wrote:
>> On 09/25/2013 09:47 AM, Chen Gang wrote:
>>> On 09/25/2013 09:14 AM, Tejun Heo wrote:
>>>> On Wed, Sep 25, 2013 at 09:06:52AM +0800, Chen Gang wrote:
>>>>> OK, I see, the 'root cause' is: "you are not the related maintainer
>>>>> either", so it is really necessary for me to spend additional time
>>>>> resource on it :-(.
>>>>
>>>> Yeah, at least partly. That and the fact that I'm not too willing to
>>>> dig into the code without further evidence. It isn't anything strange
>>>> to ask tho and I'm likely to do that even for subsystems that I know
>>>> intimately if the subject code has been stable / stale for years and
>>>> the analysis doesn't seem immediately convincing. And, if my
>>>> experience is anything to go by, it's not too unlikely that you might
>>>> hit something which doesn't agree with your current assumptions while
>>>> trying to actually trigger the problem.
>>>>
>>>> Thanks.
>>>>
>
> Oh, not cause issue, the reason is "'groups' exports extern variable
> 'init_groups', when start process, default 'cred' will set it to be
> sure of groups always be initialized".
>
> Hmm... but after all, I still think this file need be improved: "remove
> the group_info checking in groups_search()", please help check, thanks.
>
> -------------------------------patch begin------------------------------
>
> kernel/groups.c: remove useless "!group_info" checking in groups_search().
>
> Since groups_free() need not check 'group_info', groups_search() need
> not, either, and then in_group_p() and in_egroup_p(), either.
>
>
> 'groups' assumes kernel mode callers are sure of 'group_info' valid.
>
Oh, need use "callers" instead of "kernel mode callers".
> When process starts, the related kernel mode caller need set default
> 'group_info' firstly (extern variable 'init_group').
>
And also need append one sentence: "and the callers also need be sure
of "&init_group" is not passed to groups_free()."
> If groups_alloc() fails, the caller must not call any related API again
> with the related invalid 'group_info'.
>
>
> Signed-off-by: Chen Gang <[email protected]>
> ---
> kernel/groups.c | 3 ---
> 1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/groups.c b/kernel/groups.c
> index 90cf1c3..0a7f81d 100644
> --- a/kernel/groups.c
> +++ b/kernel/groups.c
> @@ -136,9 +136,6 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
> {
> unsigned int left, right;
>
> - if (!group_info)
> - return 0;
> -
> left = 0;
> right = group_info->ngroups;
> while (left < right) {
>
--
Chen Gang
--
Chen Gang
Hello, Chen.
On Thu, Sep 26, 2013 at 02:30:31PM +0800, Chen Gang wrote:
> > Oh, not cause issue, the reason is "'groups' exports extern variable
> > 'init_groups', when start process, default 'cred' will set it to be
> > sure of groups always be initialized".
> >
> > Hmm... but after all, I still think this file need be improved: "remove
> > the group_info checking in groups_search()", please help check, thanks.
Given the track record upto this point and how marginal the benefit of
the suggested change is, I'd rather not. It's quite possible that we
had specific issue where that extra conditional is necessary and I
don't feel comfortable trusting your evaluation and analysis on the
subject at the moment, so the benefit / risk ratio seems way off from
my pov.
> > If groups_alloc() fails, the caller must not call any related API again
> > with the related invalid 'group_info'.
> >
> >
> > Signed-off-by: Chen Gang <[email protected]>
Nacked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
On 09/26/2013 09:15 PM, Tejun Heo wrote:
> Hello, Chen.
>
> On Thu, Sep 26, 2013 at 02:30:31PM +0800, Chen Gang wrote:
>>> Oh, not cause issue, the reason is "'groups' exports extern variable
>>> 'init_groups', when start process, default 'cred' will set it to be
>>> sure of groups always be initialized".
>>>
>>> Hmm... but after all, I still think this file need be improved: "remove
>>> the group_info checking in groups_search()", please help check, thanks.
>
> Given the track record upto this point and how marginal the benefit of
> the suggested change is, I'd rather not. It's quite possible that we
> had specific issue where that extra conditional is necessary and I
> don't feel comfortable trusting your evaluation and analysis on the
> subject at the moment, so the benefit / risk ratio seems way off from
> my pov.
>
>>> If groups_alloc() fails, the caller must not call any related API again
>>> with the related invalid 'group_info'.
>>>
>>>
>>> Signed-off-by: Chen Gang <[email protected]>
>
> Nacked-by: Tejun Heo <[email protected]>
>
The caller should not 'generate' a new 'groups_info' by itself, so, if
invalid 'groups_info' should not pass to groups_free(), of cause it
should not pass groups_search(), either.
If permit passing an invalid 'groups_info' to groups_search(), caller
also can pass this invalid 'groups_info' to groups_free(), too.
The original author exported extern variable 'init_groups', that means
'groups' wants no duty to be sure 'group_info' whether valid, caller
should be sure of that (or it is caller's bug, not 'groups' bug).
In my opinion, it is enough to know 'groups' interface is inconsistent,
it needs improvement (in fact, I don't think it's a good idea to export
'init_groups', neither good to let caller sure of 'group_info' valid).
As an integrator or large source code maintainer, we cannot only depend
on testing, or tracing log, or some short directly causes; we also need
find and solve issues by checking sub-systems' interface or documents.
> Thanks.
>
Thanks.
--
Chen Gang
Hello, Chen.
On Fri, Sep 27, 2013 at 09:30:13AM +0800, Chen Gang wrote:
> As an integrator or large source code maintainer, we cannot only depend
> on testing, or tracing log, or some short directly causes; we also need
> find and solve issues by checking sub-systems' interface or documents.
Do you seriously think that you're the only one thinking the above?
You're repeating something obvious. The problem is that that's *all*
you're doing. Anyone who has *some* coding experience would know that
and you didn't move even an inch forward from there.
What you have done is obvious from your initial commit message, you
read some random piece of code, found something slightly inconsistent
with your concept of ideal construction, grepped a bit, and whipped up
that patch. It's such a perfect cliche of inexperience. It's
something immediately obvious and *exactly* why you were asked to
actually try out your hypothesis. It was supposed to serve two
purposes - either proving or disproving your patch && if your
knee-jerk patch was wrong, which I thought was likely given the
circumstances, making you think *why* you got it wrong and learn from
the experience.
When asked to actually verify your patch, you thought it was something
you were unfairly asked to do, and when your previous analysis was
shown to be wrong as so easily predicted, instead of thinking about
why you got that wrong and learning from the experience, you're now
just repeating the same crap in the opposite direction.
You're missing massive amount of steps between recognizing that
something is inconsistent and producing a proper improvement for it
and failing to recognize your shortcoming even when it failed right in
front of your face. You need to understand how the subsystem and code
actually work before making changes, study the callers and history of
the code especially if the code has been stable / stale for long
period of time, verify your assumptions using objective measures, and
present that in your patch. For a patch like this, preferably with
some risk analysis.
So, please take some time to mull over why your initial patch was
completely wrong and I didn't even have to read the code to predict
that your patch has high chance of being wrong. Now, you're doing the
*exactly* same thing in the opposite direction. You should be able to
recognize that there's something very wrong with that.
Thanks.
--
tejun
Firstly, thank you for your so much contents reply.
On 09/27/2013 11:36 AM, Tejun Heo wrote:
> Hello, Chen.
>
> On Fri, Sep 27, 2013 at 09:30:13AM +0800, Chen Gang wrote:
>> As an integrator or large source code maintainer, we cannot only depend
>> on testing, or tracing log, or some short directly causes; we also need
>> find and solve issues by checking sub-systems' interface or documents.
>
> Do you seriously think that you're the only one thinking the above?
> You're repeating something obvious. The problem is that that's *all*
> you're doing. Anyone who has *some* coding experience would know that
> and you didn't move even an inch forward from there.
>
> What you have done is obvious from your initial commit message, you
> read some random piece of code, found something slightly inconsistent
> with your concept of ideal construction, grepped a bit, and whipped up
> that patch. It's such a perfect cliche of inexperience. It's
> something immediately obvious and *exactly* why you were asked to
> actually try out your hypothesis. It was supposed to serve two
> purposes - either proving or disproving your patch && if your
> knee-jerk patch was wrong, which I thought was likely given the
> circumstances, making you think *why* you got it wrong and learn from
> the experience.
>
Hmm... do you mean: "can not evaluate an interface before implement(or
read details) them all"?
In my opinion
When define interface, need know most of demands (e.g. 80%), and also need know summary design, but don't need know about any implementation.
When evaluate an interface which is already defined, need know about summary demands is enough (especially, we also can reference the implementation).
Whether checking parameters belongs to interface (not only belongs to the implementation).
So, I think, at present, we have enough information to get conclusion:
"whether 'groups' interface need be improved". Before go ahead ("even
an inch"), I think it is necessary to discuss about it, firstly.
> When asked to actually verify your patch, you thought it was something
> you were unfairly asked to do, and when your previous analysis was
> shown to be wrong as so easily predicted, instead of thinking about
> why you got that wrong and learning from the experience, you're now
> just repeating the same crap in the opposite direction.
>
> You're missing massive amount of steps between recognizing that
> something is inconsistent and producing a proper improvement for it
> and failing to recognize your shortcoming even when it failed right in
> front of your face. You need to understand how the subsystem and code
> actually work before making changes, study the callers and history of
> the code especially if the code has been stable / stale for long
> period of time, verify your assumptions using objective measures, and
> present that in your patch. For a patch like this, preferably with
> some risk analysis.
>
If we are agree with each other that "this interface can be improved",
I will go ahead:
I will reference the information which Paul McKenney provided.
And also, I will use LTP's some features to give a test.
And also, I will reference some contents you said above.
Hope I can finish within next month (2013-10-31).
> So, please take some time to mull over why your initial patch was
> completely wrong and I didn't even have to read the code to predict
> that your patch has high chance of being wrong. Now, you're doing the
> *exactly* same thing in the opposite direction. You should be able to
> recognize that there's something very wrong with that.
>
No, I don't think so, in my opinion, for evaluate an api interface,
don't need see the details implementation, even don't need know all
demands.
During discussing, anyone can make mistakes, in fact, that is the main
reason why we need discussing.
Hmm... in my opinion, for evaluate one's way/method whether suitable or
not, it is not based on 1-2 mistakes, it need based on mistake/correct ratio.
> Thanks.
>
Thanks.
--
Chen Gang
Hello,
On Fri, Sep 27, 2013 at 03:16:59PM +0800, Chen Gang wrote:
> Hmm... do you mean: "can not evaluate an interface before implement(or
> read details) them all"?
No, I'm saying there are a lot more steps necessary between
recognizing that an interface needs an improvement and actually
improving it than what you're doing now.
> If we are agree with each other that "this interface can be improved",
> I will go ahead:
>
> I will reference the information which Paul McKenney provided.
> And also, I will use LTP's some features to give a test.
> And also, I will reference some contents you said above.
>
> Hope I can finish within next month (2013-10-31).
If you want to, go ahead but please see below.
> > So, please take some time to mull over why your initial patch was
> > completely wrong and I didn't even have to read the code to predict
> > that your patch has high chance of being wrong. Now, you're doing the
> > *exactly* same thing in the opposite direction. You should be able to
> > recognize that there's something very wrong with that.
>
> No, I don't think so, in my opinion, for evaluate an api interface,
> don't need see the details implementation, even don't need know all
> demands.
>
> During discussing, anyone can make mistakes, in fact, that is the main
> reason why we need discussing.
>
> Hmm... in my opinion, for evaluate one's way/method whether suitable or
> not, it is not based on 1-2 mistakes, it need based on mistake/correct ratio.
The thing is you are showing a classical and common failure pattern
which is known to lead to bad code. The only safe thing you'd be able
to do with your current pattern is making changes which are completely
contained and don't affect its interaction with large body of code,
and by not doing the necessary steps, you're shifting what you should
have done to your reviewers.
Your patch is bascially just saying "this part looks a bit
inconsistent and may need to be improved" and that's all it is. This
is bad in two ways. Firstly, the workload on reviewer is higher as
they have to do the actual work. Secondly, it's a lot more likely to
lead to bugs as the developer is supposed to be our first and best
line of defense against introducing silliness and reviewers operate on
the assumption that the developer did her role.
Please recognize that obvious local changes and changes which may
affect larger interaction are different. You will need to either
stick to obvious local changes or put a lot of effort into learning
how to do larger scope work.
I hope you understand what I mean. If not, I don't know what else I
can do. I already spent too much time on this thread and probably
won't be as verbose in my future interactions, so if you can come up
with a good patch with convincing enough presentation, go for it. If
not, I'm likely to nack it again.
Thanks.
--
tejun
On 09/27/2013 07:53 PM, Tejun Heo wrote:
> Hello,
>
> On Fri, Sep 27, 2013 at 03:16:59PM +0800, Chen Gang wrote:
>> Hmm... do you mean: "can not evaluate an interface before implement(or
>> read details) them all"?
>
> No, I'm saying there are a lot more steps necessary between
> recognizing that an interface needs an improvement and actually
> improving it than what you're doing now.
>
>> If we are agree with each other that "this interface can be improved",
>> I will go ahead:
>>
>> I will reference the information which Paul McKenney provided.
>> And also, I will use LTP's some features to give a test.
>> And also, I will reference some contents you said above.
>>
>> Hope I can finish within next month (2013-10-31).
>
> If you want to, go ahead but please see below.
>
>>> So, please take some time to mull over why your initial patch was
>>> completely wrong and I didn't even have to read the code to predict
>>> that your patch has high chance of being wrong. Now, you're doing the
>>> *exactly* same thing in the opposite direction. You should be able to
>>> recognize that there's something very wrong with that.
>>
>> No, I don't think so, in my opinion, for evaluate an api interface,
>> don't need see the details implementation, even don't need know all
>> demands.
>>
>> During discussing, anyone can make mistakes, in fact, that is the main
>> reason why we need discussing.
>>
>> Hmm... in my opinion, for evaluate one's way/method whether suitable or
>> not, it is not based on 1-2 mistakes, it need based on mistake/correct ratio.
>
> The thing is you are showing a classical and common failure pattern
> which is known to lead to bad code. The only safe thing you'd be able
> to do with your current pattern is making changes which are completely
> contained and don't affect its interaction with large body of code,
> and by not doing the necessary steps, you're shifting what you should
> have done to your reviewers.
>
> Your patch is bascially just saying "this part looks a bit
> inconsistent and may need to be improved" and that's all it is. This
> is bad in two ways. Firstly, the workload on reviewer is higher as
> they have to do the actual work. Secondly, it's a lot more likely to
> lead to bugs as the developer is supposed to be our first and best
> line of defense against introducing silliness and reviewers operate on
> the assumption that the developer did her role.
>
> Please recognize that obvious local changes and changes which may
> affect larger interaction are different. You will need to either
> stick to obvious local changes or put a lot of effort into learning
> how to do larger scope work.
>
Do we agree with each other:
Current 'groups' interface need be improved, although maybe my 2 fix patches are incorrect (but also maybe one of them is correct).
And we need additional steps to find the correct fix.
If so, I should continue, or I think we still need discussing.
> I hope you understand what I mean. If not, I don't know what else I
> can do. I already spent too much time on this thread and probably
> won't be as verbose in my future interactions, so if you can come up
> with a good patch with convincing enough presentation, go for it. If
> not, I'm likely to nack it again.
>
Hmm... I can understand your feelings. :-)
> Thanks.
>
Thanks.
--
Chen Gang
On 09/27/2013 08:19 PM, Chen Gang wrote:
> On 09/27/2013 07:53 PM, Tejun Heo wrote:
>> Hello,
>>
>> On Fri, Sep 27, 2013 at 03:16:59PM +0800, Chen Gang wrote:
>>> Hmm... do you mean: "can not evaluate an interface before implement(or
>>> read details) them all"?
>>
>> No, I'm saying there are a lot more steps necessary between
>> recognizing that an interface needs an improvement and actually
>> improving it than what you're doing now.
>>
>>> If we are agree with each other that "this interface can be improved",
>>> I will go ahead:
>>>
>>> I will reference the information which Paul McKenney provided.
>>> And also, I will use LTP's some features to give a test.
>>> And also, I will reference some contents you said above.
>>>
>>> Hope I can finish within next month (2013-10-31).
>>
>> If you want to, go ahead but please see below.
>>
>>>> So, please take some time to mull over why your initial patch was
>>>> completely wrong and I didn't even have to read the code to predict
>>>> that your patch has high chance of being wrong. Now, you're doing the
>>>> *exactly* same thing in the opposite direction. You should be able to
>>>> recognize that there's something very wrong with that.
>>>
>>> No, I don't think so, in my opinion, for evaluate an api interface,
>>> don't need see the details implementation, even don't need know all
>>> demands.
>>>
>>> During discussing, anyone can make mistakes, in fact, that is the main
>>> reason why we need discussing.
>>>
>>> Hmm... in my opinion, for evaluate one's way/method whether suitable or
>>> not, it is not based on 1-2 mistakes, it need based on mistake/correct ratio.
>>
>> The thing is you are showing a classical and common failure pattern
>> which is known to lead to bad code. The only safe thing you'd be able
>> to do with your current pattern is making changes which are completely
>> contained and don't affect its interaction with large body of code,
>> and by not doing the necessary steps, you're shifting what you should
>> have done to your reviewers.
>>
>> Your patch is bascially just saying "this part looks a bit
>> inconsistent and may need to be improved" and that's all it is. This
>> is bad in two ways. Firstly, the workload on reviewer is higher as
>> they have to do the actual work. Secondly, it's a lot more likely to
>> lead to bugs as the developer is supposed to be our first and best
>> line of defense against introducing silliness and reviewers operate on
>> the assumption that the developer did her role.
>>
>> Please recognize that obvious local changes and changes which may
>> affect larger interaction are different. You will need to either
>> stick to obvious local changes or put a lot of effort into learning
>> how to do larger scope work.
>>
>
> Do we agree with each other:
>
> Current 'groups' interface need be improved, although maybe my 2 fix patches are incorrect (but also maybe one of them is correct).
> And we need additional steps to find the correct fix.
>
> If so, I should continue, or I think we still need discussing.
>
>
>> I hope you understand what I mean. If not, I don't know what else I
>> can do. I already spent too much time on this thread and probably
>> won't be as verbose in my future interactions, so if you can come up
>> with a good patch with convincing enough presentation, go for it. If
>> not, I'm likely to nack it again.
>>
>
> Hmm... I can understand your feelings. :-)
>
>> Thanks.
>>
Hmm... excuse me, before getting agreement, I can not "go an inch". And
I think it is still valuable to discuss about it before "go an inch".
How about use WARN_ON() on "!group_info" for groups_search()? It can
let 'groups' interface 'explains' itself 'reasonably' (maybe the 3rd
'patch' is just bothering you? ;-) ).
-------------------------------patch begin------------------------------
kernel/groups.c: add WARN_ON() on "!group_info" for groups_search()
'groups' interface assumes caller need be sure of 'group_info' valid
(if 'group_info' is not allocated, it need point to 'init_group').
If callers pass invalid 'group_info' to groups_search(), we can sure
"current usage is incorrect, and need be fixed", although we can not
sure "in current condition, OS must be continuing blindly".
So need add WARN_ON (not BUG_ON) in groups_search() for alerting.
Signed-off-by: Chen Gang <[email protected]>
---
kernel/groups.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/groups.c b/kernel/groups.c
index 90cf1c3..d201da0 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -136,7 +136,7 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
{
unsigned int left, right;
- if (!group_info)
+ if (WARN_ON(!group_info))
return 0;
left = 0;
--
1.7.7.6
-------------------------------patch end--------------------------------
>
>
> Thanks.
>
--
Chen Gang