| Message ID | X9NGZWnZl5/Mt99R@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hugetlb: Fix an error code in hugetlb_reserve_pages() | expand |
Hi Dan, On Fri, Dec 11, 2020 at 3:44 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > Preserve the error code from region_add() instead of returning success. > > Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > From static analysis. Untested. > > mm/hugetlb.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 1f3bf1710b66..ac2e48b9f1d7 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode, > > if (unlikely(add < 0)) { > hugetlb_acct_memory(h, -gbl_reserve); > + ret = add; This function returns int but ret is long type. Does it need correction ? > goto out_put_pages; > } else if (unlikely(chg > add)) { > /* > -- > 2.29.2 > >
On Sat, Dec 12, 2020 at 01:19:28AM +0530, Souptick Joarder wrote: > > @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode, > > > > if (unlikely(add < 0)) { > > hugetlb_acct_memory(h, -gbl_reserve); > > + ret = add; > > This function returns int but ret is long type. > Does it need correction ? I wouold say "no", because 'ret' isn't returned _by_ this function (*), its purpose is to capture the return value from other functions. (*) OK, it is, if ret < 0. But ret < 0 really means "Is this an error number", which can be perfectly well represented in an int, short or long. char is too small ;-) So the range of values which will be placed in 'ret' is (-4096-LONG_MAX]
On Sat, Dec 12, 2020 at 01:19:28AM +0530, Souptick Joarder wrote: > Hi Dan, > > On Fri, Dec 11, 2020 at 3:44 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > Preserve the error code from region_add() instead of returning success. > > > > Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > From static analysis. Untested. > > > > mm/hugetlb.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index 1f3bf1710b66..ac2e48b9f1d7 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode, > > > > if (unlikely(add < 0)) { > > hugetlb_acct_memory(h, -gbl_reserve); > > + ret = add; > > This function returns int but ret is long type. > Does it need correction ? > It doesn't *need* correction. The code works fine as-is. Smatch parses it correctly, also. Hard to say if making it "ret" makes the code simpler or more complicated for a human reader. regards, dan carpenter
On 11.12.20 11:13, Dan Carpenter wrote: > Preserve the error code from region_add() instead of returning success. > > Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > From static analysis. Untested. > > mm/hugetlb.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 1f3bf1710b66..ac2e48b9f1d7 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode, > > if (unlikely(add < 0)) { > hugetlb_acct_memory(h, -gbl_reserve); > + ret = add; > goto out_put_pages; > } else if (unlikely(chg > add)) { > /* > Was wioing if something like return ret ? ret : add; would be cleaner, but then I spotted "chg" ... Reviewed-by: David Hildenbrand <david@redhat.com>
On 12/11/20 2:13 AM, Dan Carpenter wrote: > Preserve the error code from region_add() instead of returning success. > > Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > From static analysis. Untested. Thanks Dan. > > mm/hugetlb.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 1f3bf1710b66..ac2e48b9f1d7 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode, > > if (unlikely(add < 0)) { > hugetlb_acct_memory(h, -gbl_reserve); > + ret = add; > goto out_put_pages; > } else if (unlikely(chg > add)) { > /* > That error path is VERY unlikely to be taken, but is indeed incorrect. When looking at this, I noticed that callers of hugetlb_reserve_pages only check for 0 or !0. This changed as the code evolved to add reservation cgroup support. The routine type can be changed to a bool and simplified some. I'll send that as a follow up patch not for stable. Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 1f3bf1710b66..ac2e48b9f1d7 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -5113,6 +5113,7 @@ int hugetlb_reserve_pages(struct inode *inode, if (unlikely(add < 0)) { hugetlb_acct_memory(h, -gbl_reserve); + ret = add; goto out_put_pages; } else if (unlikely(chg > add)) { /*
Preserve the error code from region_add() instead of returning success. Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- From static analysis. Untested. mm/hugetlb.c | 1 + 1 file changed, 1 insertion(+)