| Message ID | 20190612072842.99285-3-galpress@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | EFA fixes 2019-06-12 | expand |
On Wed, Jun 12, 2019 at 10:28:42AM +0300, Gal Pressman wrote: > When inserting a new mmap entry to the xarray we should check for > 'mmap_page' overflow as it is limited to 32 bits. > > While at it, make sure to advance the mmap_page stored on the ucontext > only after a successful insertion. > > Fixes: 40909f664d27 ("RDMA/efa: Add EFA verbs implementation") > Signed-off-by: Gal Pressman <galpress@amazon.com> > drivers/infiniband/hw/efa/efa_verbs.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c > index 0fea5d63fdbe..c463c683ae84 100644 > +++ b/drivers/infiniband/hw/efa/efa_verbs.c > @@ -204,6 +204,7 @@ static u64 mmap_entry_insert(struct efa_dev *dev, struct efa_ucontext *ucontext, > void *obj, u64 address, u64 length, u8 mmap_flag) > { > struct efa_mmap_entry *entry; > + u32 next_mmap_page; > int err; > > entry = kmalloc(sizeof(*entry), GFP_KERNEL); > @@ -216,15 +217,19 @@ static u64 mmap_entry_insert(struct efa_dev *dev, struct efa_ucontext *ucontext, > entry->mmap_flag = mmap_flag; > > xa_lock(&ucontext->mmap_xa); > + if (check_add_overflow(ucontext->mmap_xa_page, > + (u32)(length >> PAGE_SHIFT), > + &next_mmap_page)) > + goto err_unlock; > + > entry->mmap_page = ucontext->mmap_xa_page; > - ucontext->mmap_xa_page += DIV_ROUND_UP(length, PAGE_SIZE); > err = __xa_insert(&ucontext->mmap_xa, entry->mmap_page, entry, > GFP_KERNEL); > + if (err) > + goto err_unlock; > + > + ucontext->mmap_xa_page = next_mmap_page; This is not ordered right anymore, the xa_lock can be released inside __xa_insert, so to be atomic you must do everything before calling __xa_insert. Jason
On 12/06/2019 15:01, Jason Gunthorpe wrote: > On Wed, Jun 12, 2019 at 10:28:42AM +0300, Gal Pressman wrote: >> When inserting a new mmap entry to the xarray we should check for >> 'mmap_page' overflow as it is limited to 32 bits. >> >> While at it, make sure to advance the mmap_page stored on the ucontext >> only after a successful insertion. >> >> Fixes: 40909f664d27 ("RDMA/efa: Add EFA verbs implementation") >> Signed-off-by: Gal Pressman <galpress@amazon.com> >> drivers/infiniband/hw/efa/efa_verbs.c | 21 ++++++++++++++++----- >> 1 file changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c >> index 0fea5d63fdbe..c463c683ae84 100644 >> +++ b/drivers/infiniband/hw/efa/efa_verbs.c >> @@ -204,6 +204,7 @@ static u64 mmap_entry_insert(struct efa_dev *dev, struct efa_ucontext *ucontext, >> void *obj, u64 address, u64 length, u8 mmap_flag) >> { >> struct efa_mmap_entry *entry; >> + u32 next_mmap_page; >> int err; >> >> entry = kmalloc(sizeof(*entry), GFP_KERNEL); >> @@ -216,15 +217,19 @@ static u64 mmap_entry_insert(struct efa_dev *dev, struct efa_ucontext *ucontext, >> entry->mmap_flag = mmap_flag; >> >> xa_lock(&ucontext->mmap_xa); >> + if (check_add_overflow(ucontext->mmap_xa_page, >> + (u32)(length >> PAGE_SHIFT), >> + &next_mmap_page)) >> + goto err_unlock; >> + >> entry->mmap_page = ucontext->mmap_xa_page; >> - ucontext->mmap_xa_page += DIV_ROUND_UP(length, PAGE_SIZE); >> err = __xa_insert(&ucontext->mmap_xa, entry->mmap_page, entry, >> GFP_KERNEL); >> + if (err) >> + goto err_unlock; >> + >> + ucontext->mmap_xa_page = next_mmap_page; > > This is not ordered right anymore, the xa_lock can be released inside > __xa_insert, so to be atomic you must do everything before calling > __xa_insert. Ah, missed the fact that __xa_insert could release the lock :\.. Thanks Jason, will bring back the mmap_xa_page assignment before the __xa_insert call and unwind it in case of __xa_insert failure.
On 12/06/2019 16:28, Gal Pressman wrote: > On 12/06/2019 15:01, Jason Gunthorpe wrote: >> On Wed, Jun 12, 2019 at 10:28:42AM +0300, Gal Pressman wrote: >>> When inserting a new mmap entry to the xarray we should check for >>> 'mmap_page' overflow as it is limited to 32 bits. >>> >>> While at it, make sure to advance the mmap_page stored on the ucontext >>> only after a successful insertion. >>> >>> Fixes: 40909f664d27 ("RDMA/efa: Add EFA verbs implementation") >>> Signed-off-by: Gal Pressman <galpress@amazon.com> >>> drivers/infiniband/hw/efa/efa_verbs.c | 21 ++++++++++++++++----- >>> 1 file changed, 16 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c >>> index 0fea5d63fdbe..c463c683ae84 100644 >>> +++ b/drivers/infiniband/hw/efa/efa_verbs.c >>> @@ -204,6 +204,7 @@ static u64 mmap_entry_insert(struct efa_dev *dev, struct efa_ucontext *ucontext, >>> void *obj, u64 address, u64 length, u8 mmap_flag) >>> { >>> struct efa_mmap_entry *entry; >>> + u32 next_mmap_page; >>> int err; >>> >>> entry = kmalloc(sizeof(*entry), GFP_KERNEL); >>> @@ -216,15 +217,19 @@ static u64 mmap_entry_insert(struct efa_dev *dev, struct efa_ucontext *ucontext, >>> entry->mmap_flag = mmap_flag; >>> >>> xa_lock(&ucontext->mmap_xa); >>> + if (check_add_overflow(ucontext->mmap_xa_page, >>> + (u32)(length >> PAGE_SHIFT), >>> + &next_mmap_page)) >>> + goto err_unlock; >>> + >>> entry->mmap_page = ucontext->mmap_xa_page; >>> - ucontext->mmap_xa_page += DIV_ROUND_UP(length, PAGE_SIZE); >>> err = __xa_insert(&ucontext->mmap_xa, entry->mmap_page, entry, >>> GFP_KERNEL); >>> + if (err) >>> + goto err_unlock; >>> + >>> + ucontext->mmap_xa_page = next_mmap_page; >> >> This is not ordered right anymore, the xa_lock can be released inside >> __xa_insert, so to be atomic you must do everything before calling >> __xa_insert. > > Ah, missed the fact that __xa_insert could release the lock :\.. > Thanks Jason, will bring back the mmap_xa_page assignment before the __xa_insert > call and unwind it in case of __xa_insert failure. On second thought, unwinding the mmap_xa_page will cause other issues.. Will drop this part.
On Wed, 2019-06-12 at 16:42 +0300, Gal Pressman wrote: > On 12/06/2019 16:28, Gal Pressman wrote: > > On 12/06/2019 15:01, Jason Gunthorpe wrote: > > > On Wed, Jun 12, 2019 at 10:28:42AM +0300, Gal Pressman wrote: > > > > When inserting a new mmap entry to the xarray we should check > > > > for > > > > 'mmap_page' overflow as it is limited to 32 bits. > > > > > > > > While at it, make sure to advance the mmap_page stored on the > > > > ucontext > > > > only after a successful insertion. > > > > > > > > Fixes: 40909f664d27 ("RDMA/efa: Add EFA verbs implementation") > > > > Signed-off-by: Gal Pressman <galpress@amazon.com> > > > > drivers/infiniband/hw/efa/efa_verbs.c | 21 ++++++++++++++++--- > > > > -- > > > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/hw/efa/efa_verbs.c > > > > b/drivers/infiniband/hw/efa/efa_verbs.c > > > > index 0fea5d63fdbe..c463c683ae84 100644 > > > > +++ b/drivers/infiniband/hw/efa/efa_verbs.c > > > > @@ -204,6 +204,7 @@ static u64 mmap_entry_insert(struct efa_dev > > > > *dev, struct efa_ucontext *ucontext, > > > > void *obj, u64 address, u64 > > > > length, u8 mmap_flag) > > > > { > > > > struct efa_mmap_entry *entry; > > > > + u32 next_mmap_page; > > > > int err; > > > > > > > > entry = kmalloc(sizeof(*entry), GFP_KERNEL); > > > > @@ -216,15 +217,19 @@ static u64 mmap_entry_insert(struct > > > > efa_dev *dev, struct efa_ucontext *ucontext, > > > > entry->mmap_flag = mmap_flag; > > > > > > > > xa_lock(&ucontext->mmap_xa); > > > > + if (check_add_overflow(ucontext->mmap_xa_page, > > > > + (u32)(length >> PAGE_SHIFT), > > > > + &next_mmap_page)) > > > > + goto err_unlock; > > > > + > > > > entry->mmap_page = ucontext->mmap_xa_page; > > > > - ucontext->mmap_xa_page += DIV_ROUND_UP(length, > > > > PAGE_SIZE); > > > > err = __xa_insert(&ucontext->mmap_xa, entry->mmap_page, > > > > entry, > > > > GFP_KERNEL); > > > > + if (err) > > > > + goto err_unlock; > > > > + > > > > + ucontext->mmap_xa_page = next_mmap_page; > > > > > > This is not ordered right anymore, the xa_lock can be released > > > inside > > > __xa_insert, so to be atomic you must do everything before > > > calling > > > __xa_insert. > > > > Ah, missed the fact that __xa_insert could release the lock :\.. > > Thanks Jason, will bring back the mmap_xa_page assignment before > > the __xa_insert > > call and unwind it in case of __xa_insert failure. > > On second thought, unwinding the mmap_xa_page will cause other > issues.. Will > drop this part. I wasn't sure what you intended to be the final patch on this one, so I just ignored it. Please post a respin of this patch that drops whatever you want dropped. Thanks.
diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c index 0fea5d63fdbe..c463c683ae84 100644 --- a/drivers/infiniband/hw/efa/efa_verbs.c +++ b/drivers/infiniband/hw/efa/efa_verbs.c @@ -204,6 +204,7 @@ static u64 mmap_entry_insert(struct efa_dev *dev, struct efa_ucontext *ucontext, void *obj, u64 address, u64 length, u8 mmap_flag) { struct efa_mmap_entry *entry; + u32 next_mmap_page; int err; entry = kmalloc(sizeof(*entry), GFP_KERNEL); @@ -216,15 +217,19 @@ static u64 mmap_entry_insert(struct efa_dev *dev, struct efa_ucontext *ucontext, entry->mmap_flag = mmap_flag; xa_lock(&ucontext->mmap_xa); + if (check_add_overflow(ucontext->mmap_xa_page, + (u32)(length >> PAGE_SHIFT), + &next_mmap_page)) + goto err_unlock; + entry->mmap_page = ucontext->mmap_xa_page; - ucontext->mmap_xa_page += DIV_ROUND_UP(length, PAGE_SIZE); err = __xa_insert(&ucontext->mmap_xa, entry->mmap_page, entry, GFP_KERNEL); + if (err) + goto err_unlock; + + ucontext->mmap_xa_page = next_mmap_page; xa_unlock(&ucontext->mmap_xa); - if (err){ - kfree(entry); - return EFA_MMAP_INVALID; - } ibdev_dbg( &dev->ibdev, @@ -232,6 +237,12 @@ static u64 mmap_entry_insert(struct efa_dev *dev, struct efa_ucontext *ucontext, entry->obj, entry->address, entry->length, get_mmap_key(entry)); return get_mmap_key(entry); + +err_unlock: + xa_unlock(&ucontext->mmap_xa); + kfree(entry); + return EFA_MMAP_INVALID; + } int efa_query_device(struct ib_device *ibdev,
When inserting a new mmap entry to the xarray we should check for 'mmap_page' overflow as it is limited to 32 bits. While at it, make sure to advance the mmap_page stored on the ucontext only after a successful insertion. Fixes: 40909f664d27 ("RDMA/efa: Add EFA verbs implementation") Signed-off-by: Gal Pressman <galpress@amazon.com> --- drivers/infiniband/hw/efa/efa_verbs.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)