kernel/groups.c: consider about NULL for 'group_info' in all related extern functions

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(-)

kernel/groups.c +9 -3

index 6b2588d..a21a4ce 100644
@@ -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;

}

kernel/groups.c +2 -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;

If this patch is correct, also need modify the man page for the return value of getgroups().

Thanks.

Quoted 62 lines · Chen Gang

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(-)

kernel/groups.c +9 -3

index 6b2588d..a21a4ce 100644
@@ -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;

}

kernel/groups.c +2 -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;

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.

Hello,

(I have no idea about this but Andrew tagged me, probably thinking it was related to cgroups, so here it goes ;)

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

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.

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 conclusion:

during kernel startup or process creation, kernel does not intend to set 'group_info'.

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.

Hello,

Quoted 3 lines · Chen Gang

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.

Quoted 6 lines

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.

..

Quoted 3 lines

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.

From reading kernel source code, kernel itself does not intend to set

Quoted 7 lines

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).

Quoted 2 lines

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.

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.

Quoted 12 lines · Chen Gang

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

Quoted 3 lines

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.

Quoted 2 lines

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.

Hello, Chen.

Quoted 5 lines · Chen Gang

Quoted 2 lines

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.

Quoted 3 lines

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 :-(.

Quoted 3 lines · Chen Gang

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.

Quoted 14 lines · Tejun Heo

Quoted 3 lines · Chen Gang

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.

Quoted 2 lines

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.

Quoted 15 lines · Chen Gang

Quoted 14 lines · Tejun Heo

Quoted 3 lines · Chen Gang

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.

Quoted 2 lines

Quoted 2 lines

Thanks.

Quoted 1 line

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).

Quoted 6 lines

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.

Quoted 16 lines · Chen Gang

Quoted 15 lines · Chen Gang

Quoted 14 lines · Tejun Heo

Quoted 3 lines · Chen Gang

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.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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(-)

kernel/groups.c +0 -3

index 90cf1c3..0a7f81d 100644
@@ -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) {

Quoted 6 lines

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.

Quoted 5 lines

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]>

Hello, Chen.

Quoted 6 lines · Chen Gang

Quoted 6 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

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.

Hello, Chen.

Quoted 3 lines · Chen Gang

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.

Hmm... do you mean: "can not evaluate an interface before implement(or read details) them all"?

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).

Quoted 5 lines

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.

Hello,

Quoted 2 lines · Chen Gang

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.

Quoted 8 lines

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.

Quoted 10 lines

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 52 lines · Tejun Heo

Hello,

Quoted 2 lines · Chen Gang

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.

Quoted 8 lines

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.

Quoted 10 lines

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

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. :-)

Quoted 2 lines

Thanks.

Thanks.