| Message ID | 1702640236-22824-1-git-send-email-quic_zijuhu@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] tty: Make tty-ldisc module loading logic easy to understand | expand |
On Fri, Dec 15, 2023 at 07:37:16PM +0800, Zijun Hu wrote: > Current tty-ldisc module loading logic within tty_ldisc_get() > is prone to mislead beginner that the module is able to be loaded > by a user without capability CAP_SYS_MODULE, add comments and > optimize the logic to make it easy to undertand. You did not "optimize" anything here, you just: > - if (!capable(CAP_SYS_MODULE) && !tty_ldisc_autoload) > + if (!tty_ldisc_autoload && !capable(CAP_SYS_MODULE)) Switched the operands around, not really doing anything at all to make it "easier" to understand, sorry. Again, please go work with developers at: > The Qualcomm Innovation Center To come up with a better change and get them to sign-off on the change before resubmitting it to us for review. thanks, greg k-h
Hi, On 12/15/2023 7:11 PM, Zijun Hu wrote: > Current tty-ldisc module loading logic within tty_ldisc_get() > is prone to mislead beginner that the module is able to be loaded > by a user without capability CAP_SYS_MODULE, add comments to make > the logic easy to undertand. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > Changes in v2: > - Remove condition checking changes > > drivers/tty/tty_ldisc.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c > index 3f68e213df1f..34526ffaccbc 100644 > --- a/drivers/tty/tty_ldisc.c > +++ b/drivers/tty/tty_ldisc.c > @@ -150,6 +150,10 @@ static struct tty_ldisc *tty_ldisc_get(struct tty_struct *tty, int disc) > */ > ldops = get_ldops(disc); > if (IS_ERR(ldops)) { > + /* > + * Always request tty-ldisc module regardless of user's > + * CAP_SYS_MODULE if autoload is enabled. > + */ Without much knowledge of this file... What the if condition below accomplishes is evident, it probably doesn't require a comment. A more useful comment would be why it does so? Thank you... -Vijay/ > if (!capable(CAP_SYS_MODULE) && !tty_ldisc_autoload) > return ERR_PTR(-EPERM); > request_module("tty-ldisc-%d", disc);
On 15. 12. 23, 15:19, Vijaya Krishna Nivarthi wrote: > Hi, > > > On 12/15/2023 7:11 PM, Zijun Hu wrote: >> Current tty-ldisc module loading logic within tty_ldisc_get() >> is prone to mislead beginner that the module is able to be loaded >> by a user without capability CAP_SYS_MODULE, add comments to make >> the logic easy to undertand. >> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >> --- >> Changes in v2: >> - Remove condition checking changes >> >> drivers/tty/tty_ldisc.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c >> index 3f68e213df1f..34526ffaccbc 100644 >> --- a/drivers/tty/tty_ldisc.c >> +++ b/drivers/tty/tty_ldisc.c >> @@ -150,6 +150,10 @@ static struct tty_ldisc *tty_ldisc_get(struct >> tty_struct *tty, int disc) >> */ >> ldops = get_ldops(disc); >> if (IS_ERR(ldops)) { >> + /* >> + * Always request tty-ldisc module regardless of user's >> + * CAP_SYS_MODULE if autoload is enabled. >> + */ > > Without much knowledge of this file... > > > What the if condition below accomplishes is evident, After a bit of thinking, sure. > it probably doesn't require a comment. I would not add a comment there at all. I would rewrite the code so it is obvious to everyone. Like: static inline bool tty_ldisc_can_autoload(void) { return capable(CAP_SYS_MODULE) || tty_ldisc_autoload; } And then: if (!tty_ldisc_can_autoload()) return ERR_PTR(-EPERM); > A more useful comment would be why it does so? From an insider, the reason is obvious. But maybe not so much for newcomers. Well, one could document the new inline above. Like: "" We allow loads for capable users or when autoloading is explicitly enabled. "" or alike... thanks,
On 12/16/2023 1:51 AM, Elliot Berman wrote: > > > On 12/15/2023 9:26 AM, Jiri Slaby wrote: >> On 15. 12. 23, 15:19, Vijaya Krishna Nivarthi wrote: >>> Hi, >>> >>> >>> On 12/15/2023 7:11 PM, Zijun Hu wrote: >>>> Current tty-ldisc module loading logic within tty_ldisc_get() >>>> is prone to mislead beginner that the module is able to be loaded >>>> by a user without capability CAP_SYS_MODULE, add comments to make >>>> the logic easy to undertand. >>>> >>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >>>> --- >>>> Changes in v2: >>>> - Remove condition checking changes >>>> >>>> drivers/tty/tty_ldisc.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c >>>> index 3f68e213df1f..34526ffaccbc 100644 >>>> --- a/drivers/tty/tty_ldisc.c >>>> +++ b/drivers/tty/tty_ldisc.c >>>> @@ -150,6 +150,10 @@ static struct tty_ldisc *tty_ldisc_get(struct tty_struct *tty, int disc) >>>> */ >>>> ldops = get_ldops(disc); >>>> if (IS_ERR(ldops)) { >>>> + /* >>>> + * Always request tty-ldisc module regardless of user's >>>> + * CAP_SYS_MODULE if autoload is enabled. >>>> + */ > > The added comment confused me more :-) > > "Request tty-ldisc if process has CAP_SYS_MODULE or autoload is enabled" > got it, please ignore my comments and changes. >>> >>> Without much knowledge of this file... >>> >>> >>> What the if condition below accomplishes is evident, >> >> After a bit of thinking, sure. >> >>> it probably doesn't require a comment. >> >> I would not add a comment there at all. I would rewrite the code so it is obvious to everyone. Like: >> >> static inline bool tty_ldisc_can_autoload(void) >> { >> return capable(CAP_SYS_MODULE) || tty_ldisc_autoload; >> } >> >> And then: >> if (!tty_ldisc_can_autoload()) >> return ERR_PTR(-EPERM); >> if you want to remain current logic, suggest think about below question: for a user without module loading permission CAP_SYS_MODULE, kernel should not allow module to be loaded for the user, even if kernel calls request_module() to load a module for the user, the loading operation will be refused by permission checking triggered by request_module(). right? i have no concern about current design if your answer is NO. it maybe be worth double checking current logic introduced by below commit if your answer is YES 7c0cca7c847e "tty: ldisc: add sysctl to prevent autoloading of ldiscs" i also don't understand why above commit will introduce extra capable(CAP_SYS_MODULE) checking. >>> A more useful comment would be why it does so? >> >> From an insider, the reason is obvious. But maybe not so much for newcomers. Well, one could document the new inline above. Like: >> "" >> We allow loads for capable users or when autoloading is explicitly enabled. >> "" >> or alike... > > I agree with Vijaya that it seems evident after a few moments of analysis, but we're > also maybe used to reading kernel code more. I don't think we should be opposed > to changes that make code easier to grok, even if they're trivial. > > If we want to make it clearer, I like Jiri's suggestion. One other thing I'd add > is to give a reference to read config LDISC_AUTOLOAD's help text. > > Zijun, > > Please send future revisions of the patch to our internal pre-submit review list > before sending to kernel.org. Qualcommers can visit go/upstream. > got it, will follow go/upstream for further patch upstream. > - Elliot
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c index 3f68e213df1f..faa125f32110 100644 --- a/drivers/tty/tty_ldisc.c +++ b/drivers/tty/tty_ldisc.c @@ -150,7 +150,11 @@ static struct tty_ldisc *tty_ldisc_get(struct tty_struct *tty, int disc) */ ldops = get_ldops(disc); if (IS_ERR(ldops)) { - if (!capable(CAP_SYS_MODULE) && !tty_ldisc_autoload) + /* + * Always request tty-ldisc module regardless of user's + * CAP_SYS_MODULE if autoload is enabled. + */ + if (!tty_ldisc_autoload && !capable(CAP_SYS_MODULE)) return ERR_PTR(-EPERM); request_module("tty-ldisc-%d", disc); ldops = get_ldops(disc);
Current tty-ldisc module loading logic within tty_ldisc_get() is prone to mislead beginner that the module is able to be loaded by a user without capability CAP_SYS_MODULE, add comments and optimize the logic to make it easy to undertand. Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> --- drivers/tty/tty_ldisc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)