Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 gang.c...@asianux.com --- 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 > > Nacked-by: Tejun Heo > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 Nacked-by: Tejun Heo Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 "_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 > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 gang.c...@asianux.com --- 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 gang.c...@asianux.com --- 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 gang.c...@asianux.com Nacked-by: Tejun Heo t...@kernel.org Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 gang.c...@asianux.com Nacked-by: Tejun Heo t...@kernel.org 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 gang.c...@asianux.com --- 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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 gang.c...@asianux.com --- 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info' in all related extern functions
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 gang.c...@asianux.com --- 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/