Re: [PATCH] staging: ion: fix corruption of ion_import_dma_buf

[Shawn Lin]

å 2015/9/11 0:44, Laura Abbott åé:
> On 09/09/2015 10:41 PM, Colin Cross wrote:
> > On Wed, Sep 9, 2015 at 10:19 AM, Laura Abbott <labbott@xxxxxxxxxx> wrote:
> > > (adding Colin and John)
> > >
> > >
> > > On 09/09/2015 12:41 AM, Shawn Lin wrote:
> > > > we found this issue but still exit in lastest kernel. Simply
> > > > keep ion_handle_create under mutex_lock to avoid this race.
> > > >
> > > > WARNING: CPU: 2 PID: 2648 at drivers/staging/android/ion/ion.c:512
> > > > ion_handle_add+0xb4/0xc0()
> > > > ion_handle_add: buffer already found.
> > > > Modules linked in: iwlmvm iwlwifi mac80211 cfg80211 compat
> > > > CPU: 2 PID: 2648 Comm: TimedEventQueue Tainted: G        W    3.14.0 #7
> > > >    00000000 00000000 9a3efd2c 80faf273 9a3efd6c 9a3efd5c 80935dc9
> > > > 811d7fd3
> > > >    9a3efd88 00000a58 812208a0 00000200 80e128d4 80e128d4 8d4ae00c
> > > > a8cd8600
> > > >    a8cd8094 9a3efd74 80935e0e 00000009 9a3efd6c 811d7fd3 9a3efd88
> > > > 9a3efd9c
> > > > Call Trace:
> > > >     [<80faf273>] dump_stack+0x48/0x69
> > > >     [<80935dc9>] warn_slowpath_common+0x79/0x90
> > > >     [<80e128d4>] ? ion_handle_add+0xb4/0xc0
> > > >     [<80e128d4>] ? ion_handle_add+0xb4/0xc0
> > > >     [<80935e0e>] warn_slowpath_fmt+0x2e/0x30
> > > >     [<80e128d4>] ion_handle_add+0xb4/0xc0
> > > >     [<80e144cc>] ion_import_dma_buf+0x8c/0x110
> > > >     [<80c517c4>] reg_init+0x364/0x7d0
> > > >     [<80993363>] ? futex_wait+0x123/0x210
> > > >     [<80992e0e>] ? get_futex_key+0x16e/0x1e0
> > > >     [<8099308f>] ? futex_wake+0x5f/0x120
> > > >     [<80c51e19>] vpu_service_ioctl+0x1e9/0x500
> > > >     [<80994aec>] ? do_futex+0xec/0x8e0
> > > >     [<80971080>] ? prepare_to_wait_event+0xc0/0xc0
> > > >     [<80c51c30>] ? reg_init+0x7d0/0x7d0
> > > >     [<80a22562>] do_vfs_ioctl+0x2d2/0x4c0
> > > >     [<80b198ad>] ? inode_has_perm.isra.41+0x2d/0x40
> > > >     [<80b199cf>] ? file_has_perm+0x7f/0x90
> > > >     [<80b1a5f7>] ? selinux_file_ioctl+0x47/0xf0
> > > >     [<80a227a8>] SyS_ioctl+0x58/0x80
> > > >     [<80fb45e8>] syscall_call+0x7/0x7
> > > >     [<80fb0000>] ? mmc_do_calc_max_discard+0xab/0xe4
> > > >
> > > > Fixes: 83271f626 ("ion: hold reference to handle...")
> > > > Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
> > > > ---
> > > >
> > > >    drivers/staging/android/ion/ion.c | 6 +++---
> > > >    1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/android/ion/ion.c
> > > > b/drivers/staging/android/ion/ion.c
> > > > index eec878e..32e7b5c 100644
> > > > --- a/drivers/staging/android/ion/ion.c
> > > > +++ b/drivers/staging/android/ion/ion.c
> > > > @@ -1179,13 +1179,13 @@ struct ion_handle *ion_import_dma_buf(struct
> > > > ion_client *client, int fd)
> > > >                  mutex_unlock(&client->lock);
> > > >                  goto end;
> > > >          }
> > > > -       mutex_unlock(&client->lock);
> > > >
> > > >          handle = ion_handle_create(client, buffer);
> > > > -       if (IS_ERR(handle))
> > > > +       if (IS_ERR(handle)) {
> > > > +               mutex_unlock(&client->lock);
> > > >                  goto end;
> > > > +       }
> > > >
> > > > -       mutex_lock(&client->lock);
> > > >          ret = ion_handle_add(client, handle);
> > > >          mutex_unlock(&client->lock);
> > > >          if (ret) {
> > >
> > > So the patch looks correct but the locking change there seems like it
> > > was
> > > added
> > > deliberately. Colin/John, do you remember why the locking for
> > > ion_import_dma_buf
> > > changed? Was there a deadlock condition somewhere?
> > >
> > > Thanks,
> > > Laura
> >
> > I can't see any reason to not hold the mutex across ion_handle_create.
> > The patch that introduced the bug
> > (83271f6262c91a49df325c52bec8f00f4de294ca, ion: hold reference to
> > handle after ion_uhandle_get) required that the mutex not be held
> > around the call to ion_handle_put, but didn't affect
> > ion_handle_create.
>
> Thanks for confirming. With that,

Thanks for reviewing this patch, Laura. :)

> Reviewed-by: Laura Abbott <labbott@xxxxxxxxxx>


--
Best Regards
Shawn Lin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/