Message ID | 1702647690-6787-1-git-send-email-quic_zijuhu@quicinc.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-1031-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:3b04:b0:fb:cd0c:d3e with SMTP id c4csp9283313dys; Fri, 15 Dec 2023 05:44:11 -0800 (PST) X-Google-Smtp-Source: AGHT+IENrLnWzkn0ToOlXxz96Fr1gvL92e8R4pdd6NawsRTQrUjPBNjQuqLHsb+RjezmN/W5QC0B X-Received: by 2002:a05:6a21:6d93:b0:191:619b:8875 with SMTP id wl19-20020a056a216d9300b00191619b8875mr4999431pzb.13.1702647851051; Fri, 15 Dec 2023 05:44:11 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1702647851; cv=none; d=google.com; s=arc-20160816; b=K9FtRbTndS/CPqZucCZ5Dnj8OmERG+2GEUtD1+7nxTpaTvFODfB7TOOAccPabqe3dS ZgE2gDoiezsNirDdos1MZs0Wr7XB0vyWrpG0bTs/WNz+D9IW03lu4mZkf7IP3mUQOaiG TWWFktTb0aG4JIbGs8MtRrg+UeeLvJSP3qramqBAaRjROlVHUvQFVxiqV7kO0f9gOzWG Orqs/fUe9Y/6hc1vrqZVzA31DSs+BbO2asdUBWIhzAE6O0mWKT9H7nvfXuZOVcanZ6Ko yt0w554ODbG6D4/3gGtM0PvRk7REwbHzBj+Ja8jo4SkbD3AQsZ6TaJChXrD4nAAhlaKs qE9A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=mime-version:list-unsubscribe:list-subscribe:list-id:precedence :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=e1V6io6xw+X48KEN9Lw2Dc3hZe8mKdB3X54mQjBDkM8=; fh=uTLyVvPCoWbuaLGaJ164u9ro+jvjsQnhJaSLhEBaLcA=; b=cpXk/KWvFIG8d4b1EOPyj72rEbE3D1nCztJeuw6ES8oxP0rg/pj38ghdG7zaMvKUqe mPg2iLiy7HE+PN0s/iXQKJBfzAN0RC921ullm++kVND6mP07VQEFWKt610/bZ9m5Az+x /wLPbdrgaNZk3QeD793r9MrY3TgxodBBXNAkDFbB/xE7DYY/F+beReEB+/5P8bYGJaVX 77nhH/fcBr1NTcSvkpK5x3f0cRPcqowH+TPtmZVgRwGYYZe1AjKt8hGoKKTuu2Ym6IES Hb2kNw1zCAsYLWgIh7BhQZmA8ZK1KIEyrHIZT9RsB77/b9BH80H8ZPVDtZqf6+LQcDqr zArA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=FsTmMLgU; spf=pass (google.com: domain of linux-kernel+bounces-1031-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) smtp.mailfrom="linux-kernel+bounces-1031-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org. [147.75.48.161]) by mx.google.com with ESMTPS id s7-20020a170902988700b001d36a6c7de2si2726063plp.351.2023.12.15.05.44.10 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 15 Dec 2023 05:44:11 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-1031-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161; Authentication-Results: mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=FsTmMLgU; spf=pass (google.com: domain of linux-kernel+bounces-1031-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) smtp.mailfrom="linux-kernel+bounces-1031-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id 0C2AAB23CA0 for <ouuuleilei@gmail.com>; Fri, 15 Dec 2023 13:42:17 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 413A730356; Fri, 15 Dec 2023 13:41:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=quicinc.com header.i=@quicinc.com header.b="FsTmMLgU" X-Original-To: linux-kernel@vger.kernel.org Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E679F3032F; Fri, 15 Dec 2023 13:41:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=quicinc.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=quicinc.com Received: from pps.filterd (m0279872.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.24/8.17.1.24) with ESMTP id 3BFCdb3H003660; Fri, 15 Dec 2023 13:41:38 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-type; s=qcppdkim1; bh=e1V6io6xw+X48KEN9Lw2 Dc3hZe8mKdB3X54mQjBDkM8=; b=FsTmMLgU25BqlcNKNv2Bw1vGxWR8+puV6EOb ti1wCeWUkeiTtee8vutviBW8uy0RsL9nTIHUPH9efX53HongFJ0Qpv2AUGmyO7gc XS96RiO4kfWuowNQUzl3vupqMhZEZ8cdKNtucCNLQ50s55I/hx11rjJ0WjtkxvHr KbGGUfEFBL9lv+eUMlfNjVNkAJqvWMjjucrckJHmzalYi86I0OvjLUqobbXAg2cg 14CF2ZR8HK2lHG0MUfdAAsRZhZMWWnmtNigzwf1ea/lcFLBJ7xtrD/Wyd3th+THO zVc92t7wIbZo0LsUtkBrul2Gru1cZe1dyBDvPFNh78fyq3bVEg== Received: from nasanppmta05.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3v0hb013sx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 15 Dec 2023 13:41:38 +0000 (GMT) Received: from nasanex01a.na.qualcomm.com (nasanex01a.na.qualcomm.com [10.52.223.231]) by NASANPPMTA05.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 3BFDfb33008769 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 15 Dec 2023 13:41:37 GMT Received: from zijuhu-gv.qualcomm.com (10.80.80.8) by nasanex01a.na.qualcomm.com (10.52.223.231) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.40; Fri, 15 Dec 2023 05:41:34 -0800 From: Zijun Hu <quic_zijuhu@quicinc.com> To: <gregkh@linuxfoundation.org>, <jirislaby@kernel.org>, <quic_qiancai@quicinc.com>, <quic_arandive@quicinc.com>, <quic_saipraka@quicinc.com>, <quic_vnivarth@quicinc.com>, <quic_eberman@quicinc.com> CC: <quic_zijuhu@quicinc.com>, <linux-serial@vger.kernel.org>, <linux-kernel@vger.kernel.org> Subject: [PATCH v2] tty: Add comments for tty-ldisc module loading logic Date: Fri, 15 Dec 2023 21:41:30 +0800 Message-ID: <1702647690-6787-1-git-send-email-quic_zijuhu@quicinc.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1702640236-22824-1-git-send-email-quic_zijuhu@quicinc.com> References: <1702640236-22824-1-git-send-email-quic_zijuhu@quicinc.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nasanex01a.na.qualcomm.com (10.52.223.231) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-ORIG-GUID: 7dD9jupRyBhKhkRsDBa3w_rjy5WQsO-X X-Proofpoint-GUID: 7dD9jupRyBhKhkRsDBa3w_rjy5WQsO-X X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-12-09_01,2023-12-07_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0 malwarescore=0 priorityscore=1501 impostorscore=0 spamscore=0 mlxlogscore=999 clxscore=1011 lowpriorityscore=0 bulkscore=0 suspectscore=0 adultscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.19.0-2311290000 definitions=main-2312150092 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1785355673325412940 X-GMAIL-MSGID: 1785355673325412940 |
Series |
[v2] tty: Add comments for tty-ldisc module loading logic
|
|
Commit Message
quic_zijuhu
Dec. 15, 2023, 1:41 p.m. UTC
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(+)
Comments
On Fri, Dec 15, 2023 at 09:41:30PM +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 to make > the logic easy to undertand. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > Changes in v2: > - Remove condition checking changes As I asked for before, please get other, more experienced developers, from your company, to review and sign-off on this patch before sending it out again. I can't take this as-is, sorry. 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 12/15/2023 10:12 PM, Greg KH wrote: > On Fri, Dec 15, 2023 at 09:41:30PM +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 to make >> the logic easy to undertand. >> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >> --- >> Changes in v2: >> - Remove condition checking changes > > As I asked for before, please get other, more experienced developers, > from your company, to review and sign-off on this patch before sending > it out again. I can't take this as-is, sorry. > okay > greg k-h
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/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" >> >> 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... 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. - Elliot
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..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. + */ if (!capable(CAP_SYS_MODULE) && !tty_ldisc_autoload) return ERR_PTR(-EPERM); request_module("tty-ldisc-%d", disc);