On i686 PAE enabled machine the contiguous physical area could be large
and it can cause trimming down variables in below calculation in
read_vmcore() and mmap_vmcore():
tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
Then the real size passed down is not correct any more.
Suppose m->offset + m->size - *fpos being truncated to 0, buflen >0 then
we will get tsz = 0. It is of course not an expected result.
During our tests there are two problems caused by it:
1) read_vmcore will refuse to continue so makedumpfile fails.
2) mmap_vmcore will trigger BUG_ON() in remap_pfn_range().
Use unsigned long long in min_t instead so that the variables are not
truncated.
Signed-off-by: Baoquan He <[email protected]>
Signed-off-by: Dave Young <[email protected]>
---
v1->v2: spelling fix in patch log
fs/proc/vmcore.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
--- linux-x86.orig/fs/proc/vmcore.c
+++ linux-x86/fs/proc/vmcore.c
@@ -231,7 +231,9 @@ static ssize_t __read_vmcore(char *buffe
list_for_each_entry(m, &vmcore_list, list) {
if (*fpos < m->offset + m->size) {
- tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
+ tsz = (size_t)min_t(unsigned long long,
+ m->offset + m->size - *fpos,
+ buflen);
start = m->paddr + *fpos - m->offset;
tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
if (tmp < 0)
@@ -461,7 +463,8 @@ static int mmap_vmcore(struct file *file
if (start < m->offset + m->size) {
u64 paddr = 0;
- tsz = min_t(size_t, m->offset + m->size - start, size);
+ tsz = (size_t)min_t(unsigned long long,
+ m->offset + m->size - start, size);
paddr = m->paddr + start - m->offset;
if (vmcore_remap_oldmem_pfn(vma, vma->vm_start + len,
paddr >> PAGE_SHIFT, tsz,
On Fri, 11 Mar 2016 16:42:48 +0800 Dave Young <[email protected]> wrote:
> On i686 PAE enabled machine the contiguous physical area could be large
> and it can cause trimming down variables in below calculation in
> read_vmcore() and mmap_vmcore():
>
> tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
>
> Then the real size passed down is not correct any more.
> Suppose m->offset + m->size - *fpos being truncated to 0, buflen >0 then
> we will get tsz = 0. It is of course not an expected result.
I don't really understand this.
vmcore.offset if loff_t which is 64-bit
vmcore.size is long long
*fpos is loff_t
so the expression should all be done with 64-bit arithmetic anyway.
Maybe buflen (size_t) has the wrong type, but the result of the other
expression should be in-range by the time we come to doing the
comparison.
> During our tests there are two problems caused by it:
> 1) read_vmcore will refuse to continue so makedumpfile fails.
> 2) mmap_vmcore will trigger BUG_ON() in remap_pfn_range().
>
> Use unsigned long long in min_t instead so that the variables are not
> truncated.
>
> Signed-off-by: Baoquan He <[email protected]>
> Signed-off-by: Dave Young <[email protected]>
I think we'll need a cc:stable here.
> --- linux-x86.orig/fs/proc/vmcore.c
> +++ linux-x86/fs/proc/vmcore.c
> @@ -231,7 +231,9 @@ static ssize_t __read_vmcore(char *buffe
>
> list_for_each_entry(m, &vmcore_list, list) {
> if (*fpos < m->offset + m->size) {
> - tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> + tsz = (size_t)min_t(unsigned long long,
> + m->offset + m->size - *fpos,
> + buflen);
This is rather a mess. Can we please try to fix this bug by choosing
appropriate types rather than all the typecasting?
> start = m->paddr + *fpos - m->offset;
> tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
> if (tmp < 0)
> @@ -461,7 +463,8 @@ static int mmap_vmcore(struct file *file
> if (start < m->offset + m->size) {
> u64 paddr = 0;
>
> - tsz = min_t(size_t, m->offset + m->size - start, size);
> + tsz = (size_t)min_t(unsigned long long,
> + m->offset + m->size - start, size);
> paddr = m->paddr + start - m->offset;
> if (vmcore_remap_oldmem_pfn(vma, vma->vm_start + len,
> paddr >> PAGE_SHIFT, tsz,
Hi, Andrew
On 03/11/16 at 12:27pm, Andrew Morton wrote:
> On Fri, 11 Mar 2016 16:42:48 +0800 Dave Young <[email protected]> wrote:
>
> > On i686 PAE enabled machine the contiguous physical area could be large
> > and it can cause trimming down variables in below calculation in
> > read_vmcore() and mmap_vmcore():
> >
> > tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> >
> > Then the real size passed down is not correct any more.
> > Suppose m->offset + m->size - *fpos being truncated to 0, buflen >0 then
> > we will get tsz = 0. It is of course not an expected result.
>
> I don't really understand this.
>
> vmcore.offset if loff_t which is 64-bit
> vmcore.size is long long
> *fpos is loff_t
>
> so the expression should all be done with 64-bit arithmetic anyway.
#define min_t(type, x, y) ({ \
type __min1 = (x); \
type __min2 = (y); \
__min1 < __min2 ? __min1: __min2; })
Here x = m->offset + m->size - *fpos; the expression is done with 64bit
arithmetic, it is true. But x will be cast to size_t then compare x with y
The casting will cause problem.
>
> Maybe buflen (size_t) has the wrong type, but the result of the other
> expression should be in-range by the time we come to doing the
> comparison.
>
> > During our tests there are two problems caused by it:
> > 1) read_vmcore will refuse to continue so makedumpfile fails.
> > 2) mmap_vmcore will trigger BUG_ON() in remap_pfn_range().
> >
> > Use unsigned long long in min_t instead so that the variables are not
> > truncated.
> >
> > Signed-off-by: Baoquan He <[email protected]>
> > Signed-off-by: Dave Young <[email protected]>
>
> I think we'll need a cc:stable here.
Agreed. Do you think I need repost for this?
>
> > --- linux-x86.orig/fs/proc/vmcore.c
> > +++ linux-x86/fs/proc/vmcore.c
> > @@ -231,7 +231,9 @@ static ssize_t __read_vmcore(char *buffe
> >
> > list_for_each_entry(m, &vmcore_list, list) {
> > if (*fpos < m->offset + m->size) {
> > - tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> > + tsz = (size_t)min_t(unsigned long long,
> > + m->offset + m->size - *fpos,
> > + buflen);
>
> This is rather a mess. Can we please try to fix this bug by choosing
> appropriate types rather than all the typecasting?
file read/mmap buflen is size_t, so tsz is alwyas less then buflen unless
m->offset + m->size - *fpos < buflen. The only problem is we need avoid large
value of m->offset + m->size - *fpos being casted thus it will mistakenly be
less than buflen.
>
>
> > start = m->paddr + *fpos - m->offset;
> > tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
> > if (tmp < 0)
> > @@ -461,7 +463,8 @@ static int mmap_vmcore(struct file *file
> > if (start < m->offset + m->size) {
> > u64 paddr = 0;
> >
> > - tsz = min_t(size_t, m->offset + m->size - start, size);
> > + tsz = (size_t)min_t(unsigned long long,
> > + m->offset + m->size - start, size);
> > paddr = m->paddr + start - m->offset;
> > if (vmcore_remap_oldmem_pfn(vma, vma->vm_start + len,
> > paddr >> PAGE_SHIFT, tsz,
Thanks
Dave
On 2016/03/12 at 12:49, Dave Young wrote:
> Hi, Andrew
>
> On 03/11/16 at 12:27pm, Andrew Morton wrote:
>> On Fri, 11 Mar 2016 16:42:48 +0800 Dave Young <[email protected]> wrote:
>>
>>> On i686 PAE enabled machine the contiguous physical area could be large
>>> and it can cause trimming down variables in below calculation in
>>> read_vmcore() and mmap_vmcore():
>>>
>>> tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
>>>
>>> Then the real size passed down is not correct any more.
>>> Suppose m->offset + m->size - *fpos being truncated to 0, buflen >0 then
>>> we will get tsz = 0. It is of course not an expected result.
>> I don't really understand this.
>>
>> vmcore.offset if loff_t which is 64-bit
>> vmcore.size is long long
>> *fpos is loff_t
>>
>> so the expression should all be done with 64-bit arithmetic anyway.
> #define min_t(type, x, y) ({ \
> type __min1 = (x); \
> type __min2 = (y); \
> __min1 < __min2 ? __min1: __min2; })
>
> Here x = m->offset + m->size - *fpos; the expression is done with 64bit
> arithmetic, it is true. But x will be cast to size_t then compare x with y
> The casting will cause problem.
>
>> Maybe buflen (size_t) has the wrong type, but the result of the other
>> expression should be in-range by the time we come to doing the
>> comparison.
>>
>>> During our tests there are two problems caused by it:
>>> 1) read_vmcore will refuse to continue so makedumpfile fails.
>>> 2) mmap_vmcore will trigger BUG_ON() in remap_pfn_range().
>>>
>>> Use unsigned long long in min_t instead so that the variables are not
>>> truncated.
>>>
>>> Signed-off-by: Baoquan He <[email protected]>
>>> Signed-off-by: Dave Young <[email protected]>
>> I think we'll need a cc:stable here.
> Agreed. Do you think I need repost for this?
>
>>> --- linux-x86.orig/fs/proc/vmcore.c
>>> +++ linux-x86/fs/proc/vmcore.c
>>> @@ -231,7 +231,9 @@ static ssize_t __read_vmcore(char *buffe
>>>
>>> list_for_each_entry(m, &vmcore_list, list) {
>>> if (*fpos < m->offset + m->size) {
>>> - tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
>>> + tsz = (size_t)min_t(unsigned long long,
>>> + m->offset + m->size - *fpos,
>>> + buflen);
>> This is rather a mess. Can we please try to fix this bug by choosing
>> appropriate types rather than all the typecasting?
> file read/mmap buflen is size_t, so tsz is alwyas less then buflen unless
> m->offset + m->size - *fpos < buflen. The only problem is we need avoid large
> value of m->offset + m->size - *fpos being casted thus it will mistakenly be
> less than buflen.
*
Can we use "tsz = min(m->offset + m->size - *fpos, buflen)" instead?
I think it's ok for this case (both have positive values), nothing will go wrong,
also can make the code cleaner.
Regards,
Xunlei
*
>>
>>> start = m->paddr + *fpos - m->offset;
>>> tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
>>> if (tmp < 0)
>>> @@ -461,7 +463,8 @@ static int mmap_vmcore(struct file *file
>>> if (start < m->offset + m->size) {
>>> u64 paddr = 0;
>>>
>>> - tsz = min_t(size_t, m->offset + m->size - start, size);
>>> + tsz = (size_t)min_t(unsigned long long,
>>> + m->offset + m->size - start, size);
>>> paddr = m->paddr + start - m->offset;
>>> if (vmcore_remap_oldmem_pfn(vma, vma->vm_start + len,
>>> paddr >> PAGE_SHIFT, tsz,
> Thanks
> Dave
On 03/12/16 at 08:43pm, Xunlei Pang wrote:
> On 2016/03/12 at 12:49, Dave Young wrote:
> > Hi, Andrew
> >
> > On 03/11/16 at 12:27pm, Andrew Morton wrote:
> >> On Fri, 11 Mar 2016 16:42:48 +0800 Dave Young <[email protected]> wrote:
> >>
> >>> On i686 PAE enabled machine the contiguous physical area could be large
> >>> and it can cause trimming down variables in below calculation in
> >>> read_vmcore() and mmap_vmcore():
> >>>
> >>> tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> >>>
> >>> Then the real size passed down is not correct any more.
> >>> Suppose m->offset + m->size - *fpos being truncated to 0, buflen >0 then
> >>> we will get tsz = 0. It is of course not an expected result.
> >> I don't really understand this.
> >>
> >> vmcore.offset if loff_t which is 64-bit
> >> vmcore.size is long long
> >> *fpos is loff_t
> >>
> >> so the expression should all be done with 64-bit arithmetic anyway.
> > #define min_t(type, x, y) ({ \
> > type __min1 = (x); \
> > type __min2 = (y); \
> > __min1 < __min2 ? __min1: __min2; })
> >
> > Here x = m->offset + m->size - *fpos; the expression is done with 64bit
> > arithmetic, it is true. But x will be cast to size_t then compare x with y
> > The casting will cause problem.
> >
> >> Maybe buflen (size_t) has the wrong type, but the result of the other
> >> expression should be in-range by the time we come to doing the
> >> comparison.
> >>
> >>> During our tests there are two problems caused by it:
> >>> 1) read_vmcore will refuse to continue so makedumpfile fails.
> >>> 2) mmap_vmcore will trigger BUG_ON() in remap_pfn_range().
> >>>
> >>> Use unsigned long long in min_t instead so that the variables are not
> >>> truncated.
> >>>
> >>> Signed-off-by: Baoquan He <[email protected]>
> >>> Signed-off-by: Dave Young <[email protected]>
> >> I think we'll need a cc:stable here.
> > Agreed. Do you think I need repost for this?
> >
> >>> --- linux-x86.orig/fs/proc/vmcore.c
> >>> +++ linux-x86/fs/proc/vmcore.c
> >>> @@ -231,7 +231,9 @@ static ssize_t __read_vmcore(char *buffe
> >>>
> >>> list_for_each_entry(m, &vmcore_list, list) {
> >>> if (*fpos < m->offset + m->size) {
> >>> - tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> >>> + tsz = (size_t)min_t(unsigned long long,
> >>> + m->offset + m->size - *fpos,
> >>> + buflen);
> >> This is rather a mess. Can we please try to fix this bug by choosing
> >> appropriate types rather than all the typecasting?
> > file read/mmap buflen is size_t, so tsz is alwyas less then buflen unless
> > m->offset + m->size - *fpos < buflen. The only problem is we need avoid large
> > value of m->offset + m->size - *fpos being casted thus it will mistakenly be
> > less than buflen.
>
> *
> Can we use "tsz = min(m->offset + m->size - *fpos, buflen)" instead?
> I think it's ok for this case (both have positive values), nothing will go wrong,
> also can make the code cleaner.
We can't. Macro min() has a type checking. min_t is necessary here.
>
> *
> >>
> >>> start = m->paddr + *fpos - m->offset;
> >>> tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
> >>> if (tmp < 0)
> >>> @@ -461,7 +463,8 @@ static int mmap_vmcore(struct file *file
> >>> if (start < m->offset + m->size) {
> >>> u64 paddr = 0;
> >>>
> >>> - tsz = min_t(size_t, m->offset + m->size - start, size);
> >>> + tsz = (size_t)min_t(unsigned long long,
> >>> + m->offset + m->size - start, size);
> >>> paddr = m->paddr + start - m->offset;
> >>> if (vmcore_remap_oldmem_pfn(vma, vma->vm_start + len,
> >>> paddr >> PAGE_SHIFT, tsz,
> > Thanks
> > Dave
>
On 2016/03/12 at 21:59, Baoquan He wrote:
> On 03/12/16 at 08:43pm, Xunlei Pang wrote:
>> On 2016/03/12 at 12:49, Dave Young wrote:
>>> Hi, Andrew
>>>
>>> On 03/11/16 at 12:27pm, Andrew Morton wrote:
>>>> On Fri, 11 Mar 2016 16:42:48 +0800 Dave Young <[email protected]> wrote:
>>>>
>>>>> On i686 PAE enabled machine the contiguous physical area could be large
>>>>> and it can cause trimming down variables in below calculation in
>>>>> read_vmcore() and mmap_vmcore():
>>>>>
>>>>> tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
>>>>>
>>>>> Then the real size passed down is not correct any more.
>>>>> Suppose m->offset + m->size - *fpos being truncated to 0, buflen >0 then
>>>>> we will get tsz = 0. It is of course not an expected result.
>>>> I don't really understand this.
>>>>
>>>> vmcore.offset if loff_t which is 64-bit
>>>> vmcore.size is long long
>>>> *fpos is loff_t
>>>>
>>>> so the expression should all be done with 64-bit arithmetic anyway.
>>> #define min_t(type, x, y) ({ \
>>> type __min1 = (x); \
>>> type __min2 = (y); \
>>> __min1 < __min2 ? __min1: __min2; })
>>>
>>> Here x = m->offset + m->size - *fpos; the expression is done with 64bit
>>> arithmetic, it is true. But x will be cast to size_t then compare x with y
>>> The casting will cause problem.
>>>
>>>> Maybe buflen (size_t) has the wrong type, but the result of the other
>>>> expression should be in-range by the time we come to doing the
>>>> comparison.
>>>>
>>>>> During our tests there are two problems caused by it:
>>>>> 1) read_vmcore will refuse to continue so makedumpfile fails.
>>>>> 2) mmap_vmcore will trigger BUG_ON() in remap_pfn_range().
>>>>>
>>>>> Use unsigned long long in min_t instead so that the variables are not
>>>>> truncated.
>>>>>
>>>>> Signed-off-by: Baoquan He <[email protected]>
>>>>> Signed-off-by: Dave Young <[email protected]>
>>>> I think we'll need a cc:stable here.
>>> Agreed. Do you think I need repost for this?
>>>
>>>>> --- linux-x86.orig/fs/proc/vmcore.c
>>>>> +++ linux-x86/fs/proc/vmcore.c
>>>>> @@ -231,7 +231,9 @@ static ssize_t __read_vmcore(char *buffe
>>>>>
>>>>> list_for_each_entry(m, &vmcore_list, list) {
>>>>> if (*fpos < m->offset + m->size) {
>>>>> - tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
>>>>> + tsz = (size_t)min_t(unsigned long long,
>>>>> + m->offset + m->size - *fpos,
>>>>> + buflen);
>>>> This is rather a mess. Can we please try to fix this bug by choosing
>>>> appropriate types rather than all the typecasting?
>>> file read/mmap buflen is size_t, so tsz is alwyas less then buflen unless
>>> m->offset + m->size - *fpos < buflen. The only problem is we need avoid large
>>> value of m->offset + m->size - *fpos being casted thus it will mistakenly be
>>> less than buflen.
>> *
>> Can we use "tsz = min(m->offset + m->size - *fpos, buflen)" instead?
>> I think it's ok for this case (both have positive values), nothing will go wrong,
>> also can make the code cleaner.
> We can't. Macro min() has a type checking. min_t is necessary here.
You're right, missed that :-)
Regards,
Xunlei
>
>> *
>>>>> start = m->paddr + *fpos - m->offset;
>>>>> tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
>>>>> if (tmp < 0)
>>>>> @@ -461,7 +463,8 @@ static int mmap_vmcore(struct file *file
>>>>> if (start < m->offset + m->size) {
>>>>> u64 paddr = 0;
>>>>>
>>>>> - tsz = min_t(size_t, m->offset + m->size - start, size);
>>>>> + tsz = (size_t)min_t(unsigned long long,
>>>>> + m->offset + m->size - start, size);
>>>>> paddr = m->paddr + start - m->offset;
>>>>> if (vmcore_remap_oldmem_pfn(vma, vma->vm_start + len,
>>>>> paddr >> PAGE_SHIFT, tsz,
>>> Thanks
>>> Dave
> _______________________________________________
> kexec mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/kexec
On 03/11/16 at 12:27pm, Andrew Morton wrote:
> On Fri, 11 Mar 2016 16:42:48 +0800 Dave Young <[email protected]> wrote:
>
> > On i686 PAE enabled machine the contiguous physical area could be large
> > and it can cause trimming down variables in below calculation in
> > read_vmcore() and mmap_vmcore():
> >
> > tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> >
> > Then the real size passed down is not correct any more.
> > Suppose m->offset + m->size - *fpos being truncated to 0, buflen >0 then
> > we will get tsz = 0. It is of course not an expected result.
>
> I don't really understand this.
>
> vmcore.offset if loff_t which is 64-bit
> vmcore.size is long long
> *fpos is loff_t
>
> so the expression should all be done with 64-bit arithmetic anyway.
>
> Maybe buflen (size_t) has the wrong type, but the result of the other
> expression should be in-range by the time we come to doing the
> comparison.
Hi Andrew,
these vmcore-s relates to "System RAM" area in 1st kernel. E.g on my
machine with 8G physical RAM, I got:
[bhe@x1 ~]$ grep "System RAM" /proc/iomem
00001000-0009cfff : System RAM
00100000-0fffffff : System RAM
1000b000-be0b2fff : System RAM
100000000-22dffffff : System RAM
For vmcore_list handling in mmap_vmcore() we have below formula:
tsz = min_t(size_t, m->offset + m->size - start, size);
In 2nd kernel, there could be 4 vmcore(s) to cover them for dumping
their content. For 4th area, 100000000-22dffffff, its vmcore could have
value like vmcore.offset=0x100000000, vmcore.size=0x12dffffff. 'start'
here is dynamically changed, it should begin from vmcore->offset.
While 'size' is decided in makedumpfile which is user space utility, its
value is 0x400000, this is hardcoded.
So here makedumpfile will do mmap() and dump coutinuously until the
whole area is finished. Each time it will compare 'size', namely passed
in size, with the length of left area.
For formula in read_vmcore() as Dave mentioned:
tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
I didnt' add debug printing code. From makedumpfile code it reads one
page at one time. So here the buflen should be 4K.
So here their own type is OK, but the type set in min_t is bad.
Thanks
Baoquan
>
> > During our tests there are two problems caused by it:
> > 1) read_vmcore will refuse to continue so makedumpfile fails.
> > 2) mmap_vmcore will trigger BUG_ON() in remap_pfn_range().
> >
> > Use unsigned long long in min_t instead so that the variables are not
> > truncated.
> >
> > Signed-off-by: Baoquan He <[email protected]>
> > Signed-off-by: Dave Young <[email protected]>
>
> I think we'll need a cc:stable here.
>
> > --- linux-x86.orig/fs/proc/vmcore.c
> > +++ linux-x86/fs/proc/vmcore.c
> > @@ -231,7 +231,9 @@ static ssize_t __read_vmcore(char *buffe
> >
> > list_for_each_entry(m, &vmcore_list, list) {
> > if (*fpos < m->offset + m->size) {
> > - tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> > + tsz = (size_t)min_t(unsigned long long,
> > + m->offset + m->size - *fpos,
> > + buflen);
>
> This is rather a mess. Can we please try to fix this bug by choosing
> appropriate types rather than all the typecasting?
>
>
> > start = m->paddr + *fpos - m->offset;
> > tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
> > if (tmp < 0)
> > @@ -461,7 +463,8 @@ static int mmap_vmcore(struct file *file
> > if (start < m->offset + m->size) {
> > u64 paddr = 0;
> >
> > - tsz = min_t(size_t, m->offset + m->size - start, size);
> > + tsz = (size_t)min_t(unsigned long long,
> > + m->offset + m->size - start, size);
> > paddr = m->paddr + start - m->offset;
> > if (vmcore_remap_oldmem_pfn(vma, vma->vm_start + len,
> > paddr >> PAGE_SHIFT, tsz,
>
> _______________________________________________
> kexec mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/kexec
Hi, HATAYAMA
On 03/14/16 at 12:25pm, HATAYAMA Daisuke wrote:
> From: Dave Young <[email protected]>
> Subject: [PATCH V2] proc-vmcore: wrong data type casting fix
> Date: Fri, 11 Mar 2016 16:42:48 +0800
>
> > On i686 PAE enabled machine the contiguous physical area could be large
> > and it can cause trimming down variables in below calculation in
> > read_vmcore() and mmap_vmcore():
> >
> > tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> >
> > Then the real size passed down is not correct any more.
> > Suppose m->offset + m->size - *fpos being truncated to 0, buflen >0 then
>
> That is, size_t and loff_t are defined as follows on i686:
>
> (gdb) ptype size_t
> type = unsigned int
> (gdb) ptype loff_t
> type = long long int
>
> So casting by size_t means truncating a given value by 4GB.
>
> Then, if (m->offset + m->size - *fpos) is equal to or larger than 4GB,
> and is aligned with 4GB, the resulted value is 0, and
>
> > we will get tsz = 0. It is of course not an expected result.
> >
> > During our tests there are two problems caused by it:
> > 1) read_vmcore will refuse to continue so makedumpfile fails.
> > 2) mmap_vmcore will trigger BUG_ON() in remap_pfn_range().
> >
>
> we reach these errors.
>
> If (m->offset + m->size - *fpos) is not aligned with 4GB,
> read_vmcore() or mmap_vmcore() is performed with the truncated
> non-zero value as size (of course, this is also not expected value but
> the execution doesn't result in error). Then, fpos proceeds so that
> (m->offset + m->size - *fpos) is aligned with 4GB in the next loop,
> and we after all reach the errors.
>
> I think your patch description needs a bit more detail.
I will add your extra comments into the patch description and resend it.
>
> It seems good to me that the patch itself.
Thank you.
>
> > Use unsigned long long in min_t instead so that the variables are not
> > truncated.
> >
> > Signed-off-by: Baoquan He <[email protected]>
> > Signed-off-by: Dave Young <[email protected]>
> > ---
> > v1->v2: spelling fix in patch log
> > fs/proc/vmcore.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > --- linux-x86.orig/fs/proc/vmcore.c
> > +++ linux-x86/fs/proc/vmcore.c
> > @@ -231,7 +231,9 @@ static ssize_t __read_vmcore(char *buffe
> >
> > list_for_each_entry(m, &vmcore_list, list) {
> > if (*fpos < m->offset + m->size) {
> > - tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> > + tsz = (size_t)min_t(unsigned long long,
> > + m->offset + m->size - *fpos,
> > + buflen);
> > start = m->paddr + *fpos - m->offset;
> > tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
> > if (tmp < 0)
> > @@ -461,7 +463,8 @@ static int mmap_vmcore(struct file *file
> > if (start < m->offset + m->size) {
> > u64 paddr = 0;
> >
> > - tsz = min_t(size_t, m->offset + m->size - start, size);
> > + tsz = (size_t)min_t(unsigned long long,
> > + m->offset + m->size - start, size);
> > paddr = m->paddr + start - m->offset;
> > if (vmcore_remap_oldmem_pfn(vma, vma->vm_start + len,
> > paddr >> PAGE_SHIFT, tsz,
> >
> --
> Thanks.
> HATAYAMA, Daisuke
From: Dave Young <[email protected]>
Subject: [PATCH V2] proc-vmcore: wrong data type casting fix
Date: Fri, 11 Mar 2016 16:42:48 +0800
> On i686 PAE enabled machine the contiguous physical area could be large
> and it can cause trimming down variables in below calculation in
> read_vmcore() and mmap_vmcore():
>
> tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
>
> Then the real size passed down is not correct any more.
> Suppose m->offset + m->size - *fpos being truncated to 0, buflen >0 then
That is, size_t and loff_t are defined as follows on i686:
(gdb) ptype size_t
type = unsigned int
(gdb) ptype loff_t
type = long long int
So casting by size_t means truncating a given value by 4GB.
Then, if (m->offset + m->size - *fpos) is equal to or larger than 4GB,
and is aligned with 4GB, the resulted value is 0, and
> we will get tsz = 0. It is of course not an expected result.
>
> During our tests there are two problems caused by it:
> 1) read_vmcore will refuse to continue so makedumpfile fails.
> 2) mmap_vmcore will trigger BUG_ON() in remap_pfn_range().
>
we reach these errors.
If (m->offset + m->size - *fpos) is not aligned with 4GB,
read_vmcore() or mmap_vmcore() is performed with the truncated
non-zero value as size (of course, this is also not expected value but
the execution doesn't result in error). Then, fpos proceeds so that
(m->offset + m->size - *fpos) is aligned with 4GB in the next loop,
and we after all reach the errors.
I think your patch description needs a bit more detail.
It seems good to me that the patch itself.
> Use unsigned long long in min_t instead so that the variables are not
> truncated.
>
> Signed-off-by: Baoquan He <[email protected]>
> Signed-off-by: Dave Young <[email protected]>
> ---
> v1->v2: spelling fix in patch log
> fs/proc/vmcore.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> --- linux-x86.orig/fs/proc/vmcore.c
> +++ linux-x86/fs/proc/vmcore.c
> @@ -231,7 +231,9 @@ static ssize_t __read_vmcore(char *buffe
>
> list_for_each_entry(m, &vmcore_list, list) {
> if (*fpos < m->offset + m->size) {
> - tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> + tsz = (size_t)min_t(unsigned long long,
> + m->offset + m->size - *fpos,
> + buflen);
> start = m->paddr + *fpos - m->offset;
> tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
> if (tmp < 0)
> @@ -461,7 +463,8 @@ static int mmap_vmcore(struct file *file
> if (start < m->offset + m->size) {
> u64 paddr = 0;
>
> - tsz = min_t(size_t, m->offset + m->size - start, size);
> + tsz = (size_t)min_t(unsigned long long,
> + m->offset + m->size - start, size);
> paddr = m->paddr + start - m->offset;
> if (vmcore_remap_oldmem_pfn(vma, vma->vm_start + len,
> paddr >> PAGE_SHIFT, tsz,
>
--
Thanks.
HATAYAMA, Daisuke
On 03/14/16 at 12:25pm, HATAYAMA Daisuke wrote:
> From: Dave Young <[email protected]>
> Subject: [PATCH V2] proc-vmcore: wrong data type casting fix
> Date: Fri, 11 Mar 2016 16:42:48 +0800
>
> > On i686 PAE enabled machine the contiguous physical area could be large
> > and it can cause trimming down variables in below calculation in
> > read_vmcore() and mmap_vmcore():
> >
> > tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> >
> > Then the real size passed down is not correct any more.
> > Suppose m->offset + m->size - *fpos being truncated to 0, buflen >0 then
>
> That is, size_t and loff_t are defined as follows on i686:
>
> (gdb) ptype size_t
> type = unsigned int
> (gdb) ptype loff_t
> type = long long int
>
> So casting by size_t means truncating a given value by 4GB.
>
> Then, if (m->offset + m->size - *fpos) is equal to or larger than 4GB,
> and is aligned with 4GB, the resulted value is 0, and
Truncating doesn't mean align. If (m->offset + m->size - *fpos) is
larger than 4G, E.g 0x10000000f, the truncating result will be 0xf.
>
> > we will get tsz = 0. It is of course not an expected result.
We won't always get "tsz=0", just get the lower 32 bit value.
> >
> > During our tests there are two problems caused by it:
> > 1) read_vmcore will refuse to continue so makedumpfile fails.
> > 2) mmap_vmcore will trigger BUG_ON() in remap_pfn_range().
> >
>
> we reach these errors.
>
> If (m->offset + m->size - *fpos) is not aligned with 4GB,
> read_vmcore() or mmap_vmcore() is performed with the truncated
> non-zero value as size (of course, this is also not expected value but
> the execution doesn't result in error). Then, fpos proceeds so that
> (m->offset + m->size - *fpos) is aligned with 4GB in the next loop,
> and we after all reach the errors.
>
> I think your patch description needs a bit more detail.
>
> It seems good to me that the patch itself.
>
> > Use unsigned long long in min_t instead so that the variables are not
> > truncated.
> >
> > Signed-off-by: Baoquan He <[email protected]>
> > Signed-off-by: Dave Young <[email protected]>
> > ---
> > v1->v2: spelling fix in patch log
> > fs/proc/vmcore.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > --- linux-x86.orig/fs/proc/vmcore.c
> > +++ linux-x86/fs/proc/vmcore.c
> > @@ -231,7 +231,9 @@ static ssize_t __read_vmcore(char *buffe
> >
> > list_for_each_entry(m, &vmcore_list, list) {
> > if (*fpos < m->offset + m->size) {
> > - tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> > + tsz = (size_t)min_t(unsigned long long,
> > + m->offset + m->size - *fpos,
> > + buflen);
> > start = m->paddr + *fpos - m->offset;
> > tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
> > if (tmp < 0)
> > @@ -461,7 +463,8 @@ static int mmap_vmcore(struct file *file
> > if (start < m->offset + m->size) {
> > u64 paddr = 0;
> >
> > - tsz = min_t(size_t, m->offset + m->size - start, size);
> > + tsz = (size_t)min_t(unsigned long long,
> > + m->offset + m->size - start, size);
> > paddr = m->paddr + start - m->offset;
> > if (vmcore_remap_oldmem_pfn(vma, vma->vm_start + len,
> > paddr >> PAGE_SHIFT, tsz,
> >
> --
> Thanks.
> HATAYAMA, Daisuke
On 03/14/16 at 11:47am, Baoquan He wrote:
> On 03/14/16 at 12:25pm, HATAYAMA Daisuke wrote:
> > From: Dave Young <[email protected]>
> > Subject: [PATCH V2] proc-vmcore: wrong data type casting fix
> > Date: Fri, 11 Mar 2016 16:42:48 +0800
> >
> > > On i686 PAE enabled machine the contiguous physical area could be large
> > > and it can cause trimming down variables in below calculation in
> > > read_vmcore() and mmap_vmcore():
> > >
> > > tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> > >
> > > Then the real size passed down is not correct any more.
> > > Suppose m->offset + m->size - *fpos being truncated to 0, buflen >0 then
> >
> > That is, size_t and loff_t are defined as follows on i686:
> >
> > (gdb) ptype size_t
> > type = unsigned int
> > (gdb) ptype loff_t
> > type = long long int
> >
> > So casting by size_t means truncating a given value by 4GB.
> >
> > Then, if (m->offset + m->size - *fpos) is equal to or larger than 4GB,
> > and is aligned with 4GB, the resulted value is 0, and
>
> Truncating doesn't mean align. If (m->offset + m->size - *fpos) is
> larger than 4G, E.g 0x10000000f, the truncating result will be 0xf.
>
> >
> > > we will get tsz = 0. It is of course not an expected result.
>
> We won't always get "tsz=0", just get the lower 32 bit value.
OK, didn't get you still have saying in next paragraph. Ignore this
please.
>
> > >
> > > During our tests there are two problems caused by it:
> > > 1) read_vmcore will refuse to continue so makedumpfile fails.
> > > 2) mmap_vmcore will trigger BUG_ON() in remap_pfn_range().
> > >
> >
> > we reach these errors.
> >
> > If (m->offset + m->size - *fpos) is not aligned with 4GB,
> > read_vmcore() or mmap_vmcore() is performed with the truncated
> > non-zero value as size (of course, this is also not expected value but
> > the execution doesn't result in error). Then, fpos proceeds so that
> > (m->offset + m->size - *fpos) is aligned with 4GB in the next loop,
> > and we after all reach the errors.
> >
> > I think your patch description needs a bit more detail.
> >
> > It seems good to me that the patch itself.
> >
> > > Use unsigned long long in min_t instead so that the variables are not
> > > truncated.
> > >
> > > Signed-off-by: Baoquan He <[email protected]>
> > > Signed-off-by: Dave Young <[email protected]>
> > > ---
> > > v1->v2: spelling fix in patch log
> > > fs/proc/vmcore.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > --- linux-x86.orig/fs/proc/vmcore.c
> > > +++ linux-x86/fs/proc/vmcore.c
> > > @@ -231,7 +231,9 @@ static ssize_t __read_vmcore(char *buffe
> > >
> > > list_for_each_entry(m, &vmcore_list, list) {
> > > if (*fpos < m->offset + m->size) {
> > > - tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
> > > + tsz = (size_t)min_t(unsigned long long,
> > > + m->offset + m->size - *fpos,
> > > + buflen);
> > > start = m->paddr + *fpos - m->offset;
> > > tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
> > > if (tmp < 0)
> > > @@ -461,7 +463,8 @@ static int mmap_vmcore(struct file *file
> > > if (start < m->offset + m->size) {
> > > u64 paddr = 0;
> > >
> > > - tsz = min_t(size_t, m->offset + m->size - start, size);
> > > + tsz = (size_t)min_t(unsigned long long,
> > > + m->offset + m->size - start, size);
> > > paddr = m->paddr + start - m->offset;
> > > if (vmcore_remap_oldmem_pfn(vma, vma->vm_start + len,
> > > paddr >> PAGE_SHIFT, tsz,
> > >
> > --
> > Thanks.
> > HATAYAMA, Daisuke
From: Baoquan He <[email protected]>
Subject: Re: [PATCH V2] proc-vmcore: wrong data type casting fix
Date: Mon, 14 Mar 2016 11:50:53 +0800
> On 03/14/16 at 11:47am, Baoquan He wrote:
>> On 03/14/16 at 12:25pm, HATAYAMA Daisuke wrote:
>> > From: Dave Young <[email protected]>
>> > Subject: [PATCH V2] proc-vmcore: wrong data type casting fix
>> > Date: Fri, 11 Mar 2016 16:42:48 +0800
>> >
>> > > On i686 PAE enabled machine the contiguous physical area could be large
>> > > and it can cause trimming down variables in below calculation in
>> > > read_vmcore() and mmap_vmcore():
>> > >
>> > > tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
>> > >
>> > > Then the real size passed down is not correct any more.
>> > > Suppose m->offset + m->size - *fpos being truncated to 0, buflen >0 then
>> >
>> > That is, size_t and loff_t are defined as follows on i686:
>> >
>> > (gdb) ptype size_t
>> > type = unsigned int
>> > (gdb) ptype loff_t
>> > type = long long int
>> >
>> > So casting by size_t means truncating a given value by 4GB.
>> >
>> > Then, if (m->offset + m->size - *fpos) is equal to or larger than 4GB,
>> > and is aligned with 4GB, the resulted value is 0, and
>>
>> Truncating doesn't mean align. If (m->offset + m->size - *fpos) is
>> larger than 4G, E.g 0x10000000f, the truncating result will be 0xf.
>>
Thanks for pointing out. I was confused with truncation and
alignment. I wanted to say here...
>> >
>> > > we will get tsz = 0. It is of course not an expected result.
>>
>> We won't always get "tsz=0", just get the lower 32 bit value.
truncating the upper 32 bits and just getting the lower 32 bits as you
say.
>
> OK, didn't get you still have saying in next paragraph. Ignore this
> please.
>
>>
>> > >
>> > > During our tests there are two problems caused by it:
>> > > 1) read_vmcore will refuse to continue so makedumpfile fails.
>> > > 2) mmap_vmcore will trigger BUG_ON() in remap_pfn_range().
>> > >
>> >
>> > we reach these errors.
>> >
>> > If (m->offset + m->size - *fpos) is not aligned with 4GB,
>> > read_vmcore() or mmap_vmcore() is performed with the truncated
>> > non-zero value as size (of course, this is also not expected value but
>> > the execution doesn't result in error). Then, fpos proceeds so that
>> > (m->offset + m->size - *fpos) is aligned with 4GB in the next loop,
>> > and we after all reach the errors.
>> >
>> > I think your patch description needs a bit more detail.
>> >
>> > It seems good to me that the patch itself.
>> >
>> > > Use unsigned long long in min_t instead so that the variables are not
>> > > truncated.
>> > >
>> > > Signed-off-by: Baoquan He <[email protected]>
>> > > Signed-off-by: Dave Young <[email protected]>
>> > > ---
>> > > v1->v2: spelling fix in patch log
>> > > fs/proc/vmcore.c | 7 +++++--
>> > > 1 file changed, 5 insertions(+), 2 deletions(-)
>> > >
>> > > --- linux-x86.orig/fs/proc/vmcore.c
>> > > +++ linux-x86/fs/proc/vmcore.c
>> > > @@ -231,7 +231,9 @@ static ssize_t __read_vmcore(char *buffe
>> > >
>> > > list_for_each_entry(m, &vmcore_list, list) {
>> > > if (*fpos < m->offset + m->size) {
>> > > - tsz = min_t(size_t, m->offset + m->size - *fpos, buflen);
>> > > + tsz = (size_t)min_t(unsigned long long,
>> > > + m->offset + m->size - *fpos,
>> > > + buflen);
>> > > start = m->paddr + *fpos - m->offset;
>> > > tmp = read_from_oldmem(buffer, tsz, &start, userbuf);
>> > > if (tmp < 0)
>> > > @@ -461,7 +463,8 @@ static int mmap_vmcore(struct file *file
>> > > if (start < m->offset + m->size) {
>> > > u64 paddr = 0;
>> > >
>> > > - tsz = min_t(size_t, m->offset + m->size - start, size);
>> > > + tsz = (size_t)min_t(unsigned long long,
>> > > + m->offset + m->size - start, size);
>> > > paddr = m->paddr + start - m->offset;
>> > > if (vmcore_remap_oldmem_pfn(vma, vma->vm_start + len,
>> > > paddr >> PAGE_SHIFT, tsz,
>> > >
>> > --
>> > Thanks.
>> > HATAYAMA, Daisuke
>
--
Thanks.
HATAYAMA, Daisuke