* [PATCH V2] proc-vmcore: wrong data type casting fix @ 2016-03-11 8:42 Dave Young 2016-03-11 20:27 ` Andrew Morton 2016-03-14 3:25 ` HATAYAMA Daisuke 0 siblings, 2 replies; 12+ messages in thread From: Dave Young @ 2016-03-11 8:42 UTC (permalink / raw) To: Andrew Morton, linux-kernel, d.hatayama, bhe, vgoyal, kexec Cc: dyoung, nasa4836, mhuang 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 <bhe@redhat.com> Signed-off-by: Dave Young <dyoung@redhat.com> --- 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, ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] proc-vmcore: wrong data type casting fix 2016-03-11 8:42 [PATCH V2] proc-vmcore: wrong data type casting fix Dave Young @ 2016-03-11 20:27 ` Andrew Morton 2016-03-12 4:49 ` Dave Young 2016-03-14 2:41 ` Baoquan He 2016-03-14 3:25 ` HATAYAMA Daisuke 1 sibling, 2 replies; 12+ messages in thread From: Andrew Morton @ 2016-03-11 20:27 UTC (permalink / raw) To: Dave Young; +Cc: linux-kernel, d.hatayama, bhe, vgoyal, kexec, nasa4836, mhuang On Fri, 11 Mar 2016 16:42:48 +0800 Dave Young <dyoung@redhat.com> 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 <bhe@redhat.com> > Signed-off-by: Dave Young <dyoung@redhat.com> 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, ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] proc-vmcore: wrong data type casting fix 2016-03-11 20:27 ` Andrew Morton @ 2016-03-12 4:49 ` Dave Young 2016-03-12 12:43 ` Xunlei Pang 2016-03-14 2:41 ` Baoquan He 1 sibling, 1 reply; 12+ messages in thread From: Dave Young @ 2016-03-12 4:49 UTC (permalink / raw) To: Andrew Morton Cc: linux-kernel, d.hatayama, bhe, vgoyal, kexec, nasa4836, mhuang Hi, Andrew On 03/11/16 at 12:27pm, Andrew Morton wrote: > On Fri, 11 Mar 2016 16:42:48 +0800 Dave Young <dyoung@redhat.com> 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 <bhe@redhat.com> > > Signed-off-by: Dave Young <dyoung@redhat.com> > > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] proc-vmcore: wrong data type casting fix 2016-03-12 4:49 ` Dave Young @ 2016-03-12 12:43 ` Xunlei Pang 2016-03-12 13:59 ` Baoquan He 0 siblings, 1 reply; 12+ messages in thread From: Xunlei Pang @ 2016-03-12 12:43 UTC (permalink / raw) To: Dave Young, Andrew Morton Cc: linux-kernel, d.hatayama, bhe, vgoyal, kexec, nasa4836, mhuang 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 <dyoung@redhat.com> 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 <bhe@redhat.com> >>> Signed-off-by: Dave Young <dyoung@redhat.com> >> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] proc-vmcore: wrong data type casting fix 2016-03-12 12:43 ` Xunlei Pang @ 2016-03-12 13:59 ` Baoquan He 2016-03-13 6:11 ` Xunlei Pang 0 siblings, 1 reply; 12+ messages in thread From: Baoquan He @ 2016-03-12 13:59 UTC (permalink / raw) To: xlpang Cc: Dave Young, Andrew Morton, linux-kernel, d.hatayama, vgoyal, kexec, nasa4836, mhuang 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 <dyoung@redhat.com> 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 <bhe@redhat.com> > >>> Signed-off-by: Dave Young <dyoung@redhat.com> > >> 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] proc-vmcore: wrong data type casting fix 2016-03-12 13:59 ` Baoquan He @ 2016-03-13 6:11 ` Xunlei Pang 0 siblings, 0 replies; 12+ messages in thread From: Xunlei Pang @ 2016-03-13 6:11 UTC (permalink / raw) To: Baoquan He, xlpang Cc: mhuang, kexec, linux-kernel, nasa4836, d.hatayama, Andrew Morton, Dave Young, vgoyal 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 <dyoung@redhat.com> 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 <bhe@redhat.com> >>>>> Signed-off-by: Dave Young <dyoung@redhat.com> >>>> 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 > kexec@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] proc-vmcore: wrong data type casting fix 2016-03-11 20:27 ` Andrew Morton 2016-03-12 4:49 ` Dave Young @ 2016-03-14 2:41 ` Baoquan He 1 sibling, 0 replies; 12+ messages in thread From: Baoquan He @ 2016-03-14 2:41 UTC (permalink / raw) To: Andrew Morton Cc: Dave Young, mhuang, kexec, nasa4836, linux-kernel, d.hatayama, vgoyal On 03/11/16 at 12:27pm, Andrew Morton wrote: > On Fri, 11 Mar 2016 16:42:48 +0800 Dave Young <dyoung@redhat.com> 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 <bhe@redhat.com> > > Signed-off-by: Dave Young <dyoung@redhat.com> > > 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 > kexec@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] proc-vmcore: wrong data type casting fix 2016-03-11 8:42 [PATCH V2] proc-vmcore: wrong data type casting fix Dave Young 2016-03-11 20:27 ` Andrew Morton @ 2016-03-14 3:25 ` HATAYAMA Daisuke 2016-03-14 3:31 ` Dave Young 2016-03-14 3:47 ` Baoquan He 1 sibling, 2 replies; 12+ messages in thread From: HATAYAMA Daisuke @ 2016-03-14 3:25 UTC (permalink / raw) To: dyoung; +Cc: akpm, linux-kernel, bhe, vgoyal, kexec, nasa4836, mhuang From: Dave Young <dyoung@redhat.com> 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 <bhe@redhat.com> > Signed-off-by: Dave Young <dyoung@redhat.com> > --- > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] proc-vmcore: wrong data type casting fix 2016-03-14 3:25 ` HATAYAMA Daisuke @ 2016-03-14 3:31 ` Dave Young 2016-03-14 3:47 ` Baoquan He 1 sibling, 0 replies; 12+ messages in thread From: Dave Young @ 2016-03-14 3:31 UTC (permalink / raw) To: HATAYAMA Daisuke; +Cc: akpm, linux-kernel, bhe, vgoyal, kexec, nasa4836, mhuang Hi, HATAYAMA On 03/14/16 at 12:25pm, HATAYAMA Daisuke wrote: > From: Dave Young <dyoung@redhat.com> > 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 <bhe@redhat.com> > > Signed-off-by: Dave Young <dyoung@redhat.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] proc-vmcore: wrong data type casting fix 2016-03-14 3:25 ` HATAYAMA Daisuke 2016-03-14 3:31 ` Dave Young @ 2016-03-14 3:47 ` Baoquan He 2016-03-14 3:50 ` Baoquan He 1 sibling, 1 reply; 12+ messages in thread From: Baoquan He @ 2016-03-14 3:47 UTC (permalink / raw) To: HATAYAMA Daisuke Cc: dyoung, akpm, linux-kernel, vgoyal, kexec, nasa4836, mhuang On 03/14/16 at 12:25pm, HATAYAMA Daisuke wrote: > From: Dave Young <dyoung@redhat.com> > 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 <bhe@redhat.com> > > Signed-off-by: Dave Young <dyoung@redhat.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] proc-vmcore: wrong data type casting fix 2016-03-14 3:47 ` Baoquan He @ 2016-03-14 3:50 ` Baoquan He 2016-03-14 4:36 ` HATAYAMA Daisuke 0 siblings, 1 reply; 12+ messages in thread From: Baoquan He @ 2016-03-14 3:50 UTC (permalink / raw) To: HATAYAMA Daisuke Cc: dyoung, akpm, linux-kernel, vgoyal, kexec, nasa4836, mhuang On 03/14/16 at 11:47am, Baoquan He wrote: > On 03/14/16 at 12:25pm, HATAYAMA Daisuke wrote: > > From: Dave Young <dyoung@redhat.com> > > 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 <bhe@redhat.com> > > > Signed-off-by: Dave Young <dyoung@redhat.com> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] proc-vmcore: wrong data type casting fix 2016-03-14 3:50 ` Baoquan He @ 2016-03-14 4:36 ` HATAYAMA Daisuke 0 siblings, 0 replies; 12+ messages in thread From: HATAYAMA Daisuke @ 2016-03-14 4:36 UTC (permalink / raw) To: bhe; +Cc: dyoung, akpm, linux-kernel, vgoyal, kexec, nasa4836, mhuang From: Baoquan He <bhe@redhat.com> 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 <dyoung@redhat.com> >> > 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 <bhe@redhat.com> >> > > Signed-off-by: Dave Young <dyoung@redhat.com> >> > > --- >> > > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-03-14 4:36 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-11 8:42 [PATCH V2] proc-vmcore: wrong data type casting fix Dave Young 2016-03-11 20:27 ` Andrew Morton 2016-03-12 4:49 ` Dave Young 2016-03-12 12:43 ` Xunlei Pang 2016-03-12 13:59 ` Baoquan He 2016-03-13 6:11 ` Xunlei Pang 2016-03-14 2:41 ` Baoquan He 2016-03-14 3:25 ` HATAYAMA Daisuke 2016-03-14 3:31 ` Dave Young 2016-03-14 3:47 ` Baoquan He 2016-03-14 3:50 ` Baoquan He 2016-03-14 4:36 ` HATAYAMA Daisuke
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).