diff mbox series

[rdma-next,08/12] RDMA/odp: Check for overflow when computing the umem_odp end

Message ID 20190819111710.18440-9-leon@kernel.org (mailing list archive)
State Accepted
Delegated to: Jason Gunthorpe
Headers show
Series Improvements for ODP | expand

Commit Message

Leon Romanovsky Aug. 19, 2019, 11:17 a.m. UTC 
From: Jason Gunthorpe <jgg@mellanox.com>

Since the page size can be extended in the ODP case by IB_ACCESS_HUGETLB
the existing overflow checks done by ib_umem_get() are not
sufficient. Check for overflow again.

Further, remove the unchecked math from the inlines and just use the
precomputed value stored in the interval_tree_node.

Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/umem_odp.c | 25 +++++++++++++++++++------
 include/rdma/ib_umem_odp.h         |  5 ++---
 2 files changed, 21 insertions(+), 9 deletions(-)

Comments

Nathan Chancellor Aug. 26, 2019, 4:42 p.m. UTC | #1 
On Mon, Aug 19, 2019 at 02:17:06PM +0300, Leon Romanovsky wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
> 
> Since the page size can be extended in the ODP case by IB_ACCESS_HUGETLB
> the existing overflow checks done by ib_umem_get() are not
> sufficient. Check for overflow again.
> 
> Further, remove the unchecked math from the inlines and just use the
> precomputed value stored in the interval_tree_node.
> 
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  drivers/infiniband/core/umem_odp.c | 25 +++++++++++++++++++------
>  include/rdma/ib_umem_odp.h         |  5 ++---
>  2 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> index 2575dd783196..46ae9962fae3 100644
> --- a/drivers/infiniband/core/umem_odp.c
> +++ b/drivers/infiniband/core/umem_odp.c
> @@ -294,19 +294,32 @@ static inline int ib_init_umem_odp(struct ib_umem_odp *umem_odp,
>  
>  	umem_odp->umem.is_odp = 1;
>  	if (!umem_odp->is_implicit_odp) {
> -		size_t pages = ib_umem_odp_num_pages(umem_odp);
> -
> +		size_t page_size = 1UL << umem_odp->page_shift;
> +		size_t pages;
> +
> +		umem_odp->interval_tree.start =
> +			ALIGN_DOWN(umem_odp->umem.address, page_size);
> +		if (check_add_overflow(umem_odp->umem.address,
> +				       umem_odp->umem.length,
> +				       &umem_odp->interval_tree.last))
> +			return -EOVERFLOW;

This if statement causes a warning on 32-bit ARM:

drivers/infiniband/core/umem_odp.c:295:7: warning: comparison of distinct
pointer types ('typeof (umem_odp->umem.address) *' (aka 'unsigned long *')
and 'typeof (umem_odp->umem.length) *' (aka 'unsigned int *'))
[-Wcompare-distinct-pointer-types]
                if (check_add_overflow(umem_odp->umem.address,
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/overflow.h:59:15: note: expanded from macro 'check_add_overflow'
        (void) (&__a == &__b);                  \
                ~~~~ ^  ~~~~
1 warning generated.

Cheers,
Nathan
Jason Gunthorpe Aug. 26, 2019, 4:55 p.m. UTC | #2 
On Mon, Aug 26, 2019 at 09:42:23AM -0700, Nathan Chancellor wrote:
> On Mon, Aug 19, 2019 at 02:17:06PM +0300, Leon Romanovsky wrote:
> > From: Jason Gunthorpe <jgg@mellanox.com>
> > 
> > Since the page size can be extended in the ODP case by IB_ACCESS_HUGETLB
> > the existing overflow checks done by ib_umem_get() are not
> > sufficient. Check for overflow again.
> > 
> > Further, remove the unchecked math from the inlines and just use the
> > precomputed value stored in the interval_tree_node.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> >  drivers/infiniband/core/umem_odp.c | 25 +++++++++++++++++++------
> >  include/rdma/ib_umem_odp.h         |  5 ++---
> >  2 files changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> > index 2575dd783196..46ae9962fae3 100644
> > +++ b/drivers/infiniband/core/umem_odp.c
> > @@ -294,19 +294,32 @@ static inline int ib_init_umem_odp(struct ib_umem_odp *umem_odp,
> >  
> >  	umem_odp->umem.is_odp = 1;
> >  	if (!umem_odp->is_implicit_odp) {
> > -		size_t pages = ib_umem_odp_num_pages(umem_odp);
> > -
> > +		size_t page_size = 1UL << umem_odp->page_shift;
> > +		size_t pages;
> > +
> > +		umem_odp->interval_tree.start =
> > +			ALIGN_DOWN(umem_odp->umem.address, page_size);
> > +		if (check_add_overflow(umem_odp->umem.address,
> > +				       umem_odp->umem.length,
> > +				       &umem_odp->interval_tree.last))
> > +			return -EOVERFLOW;
> 
> This if statement causes a warning on 32-bit ARM:
> 
> drivers/infiniband/core/umem_odp.c:295:7: warning: comparison of distinct
> pointer types ('typeof (umem_odp->umem.address) *' (aka 'unsigned long *')
> and 'typeof (umem_odp->umem.length) *' (aka 'unsigned int *'))
> [-Wcompare-distinct-pointer-types]
>                 if (check_add_overflow(umem_odp->umem.address,
>                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/linux/overflow.h:59:15: note: expanded from macro 'check_add_overflow'
>         (void) (&__a == &__b);                  \
>                 ~~~~ ^  ~~~~
> 1 warning generated.

Hum, I'm pretty sure 0-day has stopped running 32 bit builds or
something :\

Jason
Nathan Chancellor Aug. 27, 2019, 7:25 p.m. UTC | #3 
On Mon, Aug 26, 2019 at 04:55:45PM +0000, Jason Gunthorpe wrote:
> On Mon, Aug 26, 2019 at 09:42:23AM -0700, Nathan Chancellor wrote:
> > On Mon, Aug 19, 2019 at 02:17:06PM +0300, Leon Romanovsky wrote:
> > > From: Jason Gunthorpe <jgg@mellanox.com>
> > > 
> > > Since the page size can be extended in the ODP case by IB_ACCESS_HUGETLB
> > > the existing overflow checks done by ib_umem_get() are not
> > > sufficient. Check for overflow again.
> > > 
> > > Further, remove the unchecked math from the inlines and just use the
> > > precomputed value stored in the interval_tree_node.
> > > 
> > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > >  drivers/infiniband/core/umem_odp.c | 25 +++++++++++++++++++------
> > >  include/rdma/ib_umem_odp.h         |  5 ++---
> > >  2 files changed, 21 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> > > index 2575dd783196..46ae9962fae3 100644
> > > +++ b/drivers/infiniband/core/umem_odp.c
> > > @@ -294,19 +294,32 @@ static inline int ib_init_umem_odp(struct ib_umem_odp *umem_odp,
> > >  
> > >  	umem_odp->umem.is_odp = 1;
> > >  	if (!umem_odp->is_implicit_odp) {
> > > -		size_t pages = ib_umem_odp_num_pages(umem_odp);
> > > -
> > > +		size_t page_size = 1UL << umem_odp->page_shift;
> > > +		size_t pages;
> > > +
> > > +		umem_odp->interval_tree.start =
> > > +			ALIGN_DOWN(umem_odp->umem.address, page_size);
> > > +		if (check_add_overflow(umem_odp->umem.address,
> > > +				       umem_odp->umem.length,
> > > +				       &umem_odp->interval_tree.last))
> > > +			return -EOVERFLOW;
> > 
> > This if statement causes a warning on 32-bit ARM:
> > 
> > drivers/infiniband/core/umem_odp.c:295:7: warning: comparison of distinct
> > pointer types ('typeof (umem_odp->umem.address) *' (aka 'unsigned long *')
> > and 'typeof (umem_odp->umem.length) *' (aka 'unsigned int *'))
> > [-Wcompare-distinct-pointer-types]
> >                 if (check_add_overflow(umem_odp->umem.address,
> >                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > include/linux/overflow.h:59:15: note: expanded from macro 'check_add_overflow'
> >         (void) (&__a == &__b);                  \
> >                 ~~~~ ^  ~~~~
> > 1 warning generated.
> 
> Hum, I'm pretty sure 0-day has stopped running 32 bit builds or
> something :\
> 
> Jason

My report was with clang but GCC reports the same type of warning:

In file included from ../include/linux/slab.h:16,
                 from ../drivers/infiniband/core/umem_odp.c:38:
../drivers/infiniband/core/umem_odp.c: In function 'ib_init_umem_odp':
../include/linux/overflow.h:59:15: warning: comparison of distinct pointer types lacks a cast
   59 |  (void) (&__a == &__b);   \
      |               ^~
../drivers/infiniband/core/umem_odp.c:220:7: note: in expansion of macro 'check_add_overflow'
  220 |   if (check_add_overflow(umem_odp->umem.address,
      |       ^~~~~~~~~~~~~~~~~~

Adding Philip and Rong as I believe that they are the current 0-day
maintainers.

Cheers,
Nathan
diff mbox series

Patch

diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
index 2575dd783196..46ae9962fae3 100644
--- a/drivers/infiniband/core/umem_odp.c
+++ b/drivers/infiniband/core/umem_odp.c
@@ -294,19 +294,32 @@  static inline int ib_init_umem_odp(struct ib_umem_odp *umem_odp,
 
 	umem_odp->umem.is_odp = 1;
 	if (!umem_odp->is_implicit_odp) {
-		size_t pages = ib_umem_odp_num_pages(umem_odp);
-
+		size_t page_size = 1UL << umem_odp->page_shift;
+		size_t pages;
+
+		umem_odp->interval_tree.start =
+			ALIGN_DOWN(umem_odp->umem.address, page_size);
+		if (check_add_overflow(umem_odp->umem.address,
+				       umem_odp->umem.length,
+				       &umem_odp->interval_tree.last))
+			return -EOVERFLOW;
+		umem_odp->interval_tree.last =
+			ALIGN(umem_odp->interval_tree.last, page_size);
+		if (unlikely(umem_odp->interval_tree.last < page_size))
+			return -EOVERFLOW;
+
+		pages = (umem_odp->interval_tree.last -
+			 umem_odp->interval_tree.start) >>
+			umem_odp->page_shift;
 		if (!pages)
 			return -EINVAL;
 
 		/*
 		 * Note that the representation of the intervals in the
 		 * interval tree considers the ending point as contained in
-		 * the interval, while the function ib_umem_end returns the
-		 * first address which is not contained in the umem.
+		 * the interval.
 		 */
-		umem_odp->interval_tree.start = ib_umem_start(umem_odp);
-		umem_odp->interval_tree.last = ib_umem_end(umem_odp) - 1;
+		umem_odp->interval_tree.last--;
 
 		umem_odp->page_list = vzalloc(
 			array_size(sizeof(*umem_odp->page_list), pages));
diff --git a/include/rdma/ib_umem_odp.h b/include/rdma/ib_umem_odp.h
index 5efb67f97b0a..b37c674b7fe6 100644
--- a/include/rdma/ib_umem_odp.h
+++ b/include/rdma/ib_umem_odp.h
@@ -91,14 +91,13 @@  static inline struct ib_umem_odp *to_ib_umem_odp(struct ib_umem *umem)
 /* Returns the first page of an ODP umem. */
 static inline unsigned long ib_umem_start(struct ib_umem_odp *umem_odp)
 {
-	return ALIGN_DOWN(umem_odp->umem.address, 1UL << umem_odp->page_shift);
+	return umem_odp->interval_tree.start;
 }
 
 /* Returns the address of the page after the last one of an ODP umem. */
 static inline unsigned long ib_umem_end(struct ib_umem_odp *umem_odp)
 {
-	return ALIGN(umem_odp->umem.address + umem_odp->umem.length,
-		     1UL << umem_odp->page_shift);
+	return umem_odp->interval_tree.last + 1;
 }
 
 static inline size_t ib_umem_odp_num_pages(struct ib_umem_odp *umem_odp)