| Message ID | 20181123072135.gqvblm2vdujbvfjs@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: debug: Fix a width vs precision bug in printk | expand |
On 11/23/2018 12:51 PM, Dan Carpenter wrote: > We had intended to only print dentry->d_name.len characters but there is > a width vs precision typo so if the name isn't NUL terminated it will > read past the end of the buffer. > > Fixes: 408ddbc22be3 ("mm: print more information about mapping in __dump_page") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
On Fri 23-11-18 10:21:35, Dan Carpenter wrote: > We had intended to only print dentry->d_name.len characters but there is > a width vs precision typo so if the name isn't NUL terminated it will > read past the end of the buffer. OK, it took me quite some time to grasp what you mean here. The code works as expected because d_name.len and dname.name are in sync so there no spacing going to happen. Anyway what you propose is formally more correct I guess. > Fixes: 408ddbc22be3 ("mm: print more information about mapping in __dump_page") This sha is an unstable sha for mmotm patch. > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Acked-by: Michal Hocko <mhocko@suse.com> Thanks! > --- > mm/debug.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/debug.c b/mm/debug.c > index d18c5cea3320..faf856b652b6 100644 > --- a/mm/debug.c > +++ b/mm/debug.c > @@ -80,7 +80,7 @@ void __dump_page(struct page *page, const char *reason) > if (mapping->host->i_dentry.first) { > struct dentry *dentry; > dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias); > - pr_warn("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name); > + pr_warn("name:\"%.*s\" ", dentry->d_name.len, dentry->d_name.name); > } > } > BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1); > -- > 2.11.0 >
On 23.11.18 08:21, Dan Carpenter wrote: > We had intended to only print dentry->d_name.len characters but there is > a width vs precision typo so if the name isn't NUL terminated it will > read past the end of the buffer. > > Fixes: 408ddbc22be3 ("mm: print more information about mapping in __dump_page") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > mm/debug.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/debug.c b/mm/debug.c > index d18c5cea3320..faf856b652b6 100644 > --- a/mm/debug.c > +++ b/mm/debug.c > @@ -80,7 +80,7 @@ void __dump_page(struct page *page, const char *reason) > if (mapping->host->i_dentry.first) { > struct dentry *dentry; > dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias); > - pr_warn("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name); > + pr_warn("name:\"%.*s\" ", dentry->d_name.len, dentry->d_name.name); > } > } > BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1); > Reviewed-by: David Hildenbrand <david@redhat.com>
On Fri, Nov 23, 2018 at 10:01:25AM +0100, Michal Hocko wrote: > On Fri 23-11-18 10:21:35, Dan Carpenter wrote: > > We had intended to only print dentry->d_name.len characters but there is > > a width vs precision typo so if the name isn't NUL terminated it will > > read past the end of the buffer. > > OK, it took me quite some time to grasp what you mean here. The code > works as expected because d_name.len and dname.name are in sync so there > no spacing going to happen. Anyway what you propose is formally more > correct I guess. > Yeah. If we are sure that the name has a NUL terminator then this change has no effect. regards, dan carpenter
On 2018/11/23 23:36, Dan Carpenter wrote: > On Fri, Nov 23, 2018 at 10:01:25AM +0100, Michal Hocko wrote: >> On Fri 23-11-18 10:21:35, Dan Carpenter wrote: >>> We had intended to only print dentry->d_name.len characters but there is >>> a width vs precision typo so if the name isn't NUL terminated it will >>> read past the end of the buffer. >> >> OK, it took me quite some time to grasp what you mean here. The code >> works as expected because d_name.len and dname.name are in sync so there >> no spacing going to happen. Anyway what you propose is formally more >> correct I guess. >> > > Yeah. If we are sure that the name has a NUL terminator then this > change has no effect. There seems to be %pd which is designed for printing "struct dentry".
On Fri, 23 Nov 2018 23:48:06 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote: > On 2018/11/23 23:36, Dan Carpenter wrote: > > On Fri, Nov 23, 2018 at 10:01:25AM +0100, Michal Hocko wrote: > >> On Fri 23-11-18 10:21:35, Dan Carpenter wrote: > >>> We had intended to only print dentry->d_name.len characters but there is > >>> a width vs precision typo so if the name isn't NUL terminated it will > >>> read past the end of the buffer. > >> > >> OK, it took me quite some time to grasp what you mean here. The code > >> works as expected because d_name.len and dname.name are in sync so there > >> no spacing going to happen. Anyway what you propose is formally more > >> correct I guess. > >> > > > > Yeah. If we are sure that the name has a NUL terminator then this > > change has no effect. > > There seems to be %pd which is designed for printing "struct dentry". ooh, who knew. Can we use that please?
On Fri 23-11-18 16:08:46, Andrew Morton wrote: > On Fri, 23 Nov 2018 23:48:06 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote: > > > On 2018/11/23 23:36, Dan Carpenter wrote: > > > On Fri, Nov 23, 2018 at 10:01:25AM +0100, Michal Hocko wrote: > > >> On Fri 23-11-18 10:21:35, Dan Carpenter wrote: > > >>> We had intended to only print dentry->d_name.len characters but there is > > >>> a width vs precision typo so if the name isn't NUL terminated it will > > >>> read past the end of the buffer. > > >> > > >> OK, it took me quite some time to grasp what you mean here. The code > > >> works as expected because d_name.len and dname.name are in sync so there > > >> no spacing going to happen. Anyway what you propose is formally more > > >> correct I guess. > > >> > > > > > > Yeah. If we are sure that the name has a NUL terminator then this > > > change has no effect. > > > > There seems to be %pd which is designed for printing "struct dentry". > > ooh, who knew. Can we use that please? I wasn't aware of it either. I do not mind using it instead of the opencoded variant of mine. This should do it, right? diff --git a/mm/debug.c b/mm/debug.c index d18c5cea3320..68e9a9f2df16 100644 --- a/mm/debug.c +++ b/mm/debug.c @@ -80,7 +80,7 @@ void __dump_page(struct page *page, const char *reason) if (mapping->host->i_dentry.first) { struct dentry *dentry; dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias); - pr_warn("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name); + pr_warn("name:\"%pd\" ", dentry); } } BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
diff --git a/mm/debug.c b/mm/debug.c index d18c5cea3320..faf856b652b6 100644 --- a/mm/debug.c +++ b/mm/debug.c @@ -80,7 +80,7 @@ void __dump_page(struct page *page, const char *reason) if (mapping->host->i_dentry.first) { struct dentry *dentry; dentry = container_of(mapping->host->i_dentry.first, struct dentry, d_u.d_alias); - pr_warn("name:\"%*s\" ", dentry->d_name.len, dentry->d_name.name); + pr_warn("name:\"%.*s\" ", dentry->d_name.len, dentry->d_name.name); } } BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS + 1);
We had intended to only print dentry->d_name.len characters but there is a width vs precision typo so if the name isn't NUL terminated it will read past the end of the buffer. Fixes: 408ddbc22be3 ("mm: print more information about mapping in __dump_page") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- mm/debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)