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@xxxxxxxxxx>
> > > > 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.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57
2FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
