* [PATCH RESEND] drm/lease: fix potential race in fill_object_idr
@ 2020-03-16 7:18 Qiujun Huang
2020-03-17 17:02 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Qiujun Huang @ 2020-03-16 7:18 UTC (permalink / raw)
To: maarten.lankhorst, mripard, airlied, daniel
Cc: dri-devel, linux-kernel, Qiujun Huang
We should hold idr_mutex for idr_alloc.
Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
---
drivers/gpu/drm/drm_lease.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index b481caf..427ee21 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -418,6 +418,7 @@ static int fill_object_idr(struct drm_device *dev,
goto out_free_objects;
}
+ mutex_lock(&dev->mode_config.idr_mutex);
/* add their IDs to the lease request - taking into account
universal planes */
for (o = 0; o < object_count; o++) {
@@ -437,7 +438,7 @@ static int fill_object_idr(struct drm_device *dev,
if (ret < 0) {
DRM_DEBUG_LEASE("Object %d cannot be inserted into leases (%d)\n",
object_id, ret);
- goto out_free_objects;
+ goto out_unlock;
}
if (obj->type == DRM_MODE_OBJECT_CRTC && !universal_planes) {
struct drm_crtc *crtc = obj_to_crtc(obj);
@@ -445,20 +446,22 @@ static int fill_object_idr(struct drm_device *dev,
if (ret < 0) {
DRM_DEBUG_LEASE("Object primary plane %d cannot be inserted into leases (%d)\n",
object_id, ret);
- goto out_free_objects;
+ goto out_unlock;
}
if (crtc->cursor) {
ret = idr_alloc(leases, &drm_lease_idr_object, crtc->cursor->base.id, crtc->cursor->base.id + 1, GFP_KERNEL);
if (ret < 0) {
DRM_DEBUG_LEASE("Object cursor plane %d cannot be inserted into leases (%d)\n",
object_id, ret);
- goto out_free_objects;
+ goto out_unlock;
}
}
}
}
ret = 0;
+out_unlock:
+ mutex_unlock(&dev->mode_config.idr_mutex);
out_free_objects:
for (o = 0; o < object_count; o++) {
if (objects[o])
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] drm/lease: fix potential race in fill_object_idr
2020-03-16 7:18 [PATCH RESEND] drm/lease: fix potential race in fill_object_idr Qiujun Huang
@ 2020-03-17 17:02 ` Daniel Vetter
2020-03-17 22:33 ` Qiujun Huang
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2020-03-17 17:02 UTC (permalink / raw)
To: Qiujun Huang
Cc: maarten.lankhorst, mripard, airlied, daniel, dri-devel, linux-kernel
On Mon, Mar 16, 2020 at 03:18:23PM +0800, Qiujun Huang wrote:
> We should hold idr_mutex for idr_alloc.
>
> Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
I've not seen the first version of this anywhere in my inbox, not sure
where that got lost.
Anyway, this seems like a false positive - I'm assuming this was caught
with KCSAN. The commit message really should mention that.
fill_object_idr creates the idr, which yes is only access later on under
the idr_mutex. But here it's not yet visible to any other thread, and
hence lockless access is safe and correct.
No idea what the KCSAN complains about safe access like this best practice
is.
-Daniel
> ---
> drivers/gpu/drm/drm_lease.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index b481caf..427ee21 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -418,6 +418,7 @@ static int fill_object_idr(struct drm_device *dev,
> goto out_free_objects;
> }
>
> + mutex_lock(&dev->mode_config.idr_mutex);
> /* add their IDs to the lease request - taking into account
> universal planes */
> for (o = 0; o < object_count; o++) {
> @@ -437,7 +438,7 @@ static int fill_object_idr(struct drm_device *dev,
> if (ret < 0) {
> DRM_DEBUG_LEASE("Object %d cannot be inserted into leases (%d)\n",
> object_id, ret);
> - goto out_free_objects;
> + goto out_unlock;
> }
> if (obj->type == DRM_MODE_OBJECT_CRTC && !universal_planes) {
> struct drm_crtc *crtc = obj_to_crtc(obj);
> @@ -445,20 +446,22 @@ static int fill_object_idr(struct drm_device *dev,
> if (ret < 0) {
> DRM_DEBUG_LEASE("Object primary plane %d cannot be inserted into leases (%d)\n",
> object_id, ret);
> - goto out_free_objects;
> + goto out_unlock;
> }
> if (crtc->cursor) {
> ret = idr_alloc(leases, &drm_lease_idr_object, crtc->cursor->base.id, crtc->cursor->base.id + 1, GFP_KERNEL);
> if (ret < 0) {
> DRM_DEBUG_LEASE("Object cursor plane %d cannot be inserted into leases (%d)\n",
> object_id, ret);
> - goto out_free_objects;
> + goto out_unlock;
> }
> }
> }
> }
>
> ret = 0;
> +out_unlock:
> + mutex_unlock(&dev->mode_config.idr_mutex);
> out_free_objects:
> for (o = 0; o < object_count; o++) {
> if (objects[o])
> --
> 1.8.3.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] drm/lease: fix potential race in fill_object_idr
2020-03-17 17:02 ` Daniel Vetter
@ 2020-03-17 22:33 ` Qiujun Huang
2020-03-18 7:34 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Qiujun Huang @ 2020-03-17 22:33 UTC (permalink / raw)
To: Qiujun Huang, maarten.lankhorst, mripard, airlied, dri-devel, LKML
Cc: Daniel Vetter
On Wed, Mar 18, 2020 at 1:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Mar 16, 2020 at 03:18:23PM +0800, Qiujun Huang wrote:
> > We should hold idr_mutex for idr_alloc.
> >
> > Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
>
> I've not seen the first version of this anywhere in my inbox, not sure
> where that got lost.
>
> Anyway, this seems like a false positive - I'm assuming this was caught
> with KCSAN. The commit message really should mention that.
>
> fill_object_idr creates the idr, which yes is only access later on under
> the idr_mutex. But here it's not yet visible to any other thread, and
> hence lockless access is safe and correct.
Agree that.
Thanks.
>
> No idea what the KCSAN complains about safe access like this best practice
> is.
> -Daniel
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] drm/lease: fix potential race in fill_object_idr
2020-03-17 22:33 ` Qiujun Huang
@ 2020-03-18 7:34 ` Daniel Vetter
2020-03-18 8:09 ` Qiujun Huang
2020-03-18 12:03 ` Marco Elver
0 siblings, 2 replies; 6+ messages in thread
From: Daniel Vetter @ 2020-03-18 7:34 UTC (permalink / raw)
To: Qiujun Huang, Marco Elver
Cc: Maarten Lankhorst, Maxime Ripard, Dave Airlie, dri-devel, LKML
On Tue, Mar 17, 2020 at 11:33 PM Qiujun Huang <hqjagain@gmail.com> wrote:
>
> On Wed, Mar 18, 2020 at 1:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Mar 16, 2020 at 03:18:23PM +0800, Qiujun Huang wrote:
> > > We should hold idr_mutex for idr_alloc.
> > >
> > > Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
> >
> > I've not seen the first version of this anywhere in my inbox, not sure
> > where that got lost.
> >
> > Anyway, this seems like a false positive - I'm assuming this was caught
> > with KCSAN. The commit message really should mention that.
> >
> > fill_object_idr creates the idr, which yes is only access later on under
> > the idr_mutex. But here it's not yet visible to any other thread, and
> > hence lockless access is safe and correct.
>
> Agree that.
Do you know what the recommended annotation for kcsan false positives
like this should be? Adding kcsan author Marco.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] drm/lease: fix potential race in fill_object_idr
2020-03-18 7:34 ` Daniel Vetter
@ 2020-03-18 8:09 ` Qiujun Huang
2020-03-18 12:03 ` Marco Elver
1 sibling, 0 replies; 6+ messages in thread
From: Qiujun Huang @ 2020-03-18 8:09 UTC (permalink / raw)
To: Daniel Vetter
Cc: Marco Elver, Maarten Lankhorst, Maxime Ripard, Dave Airlie,
dri-devel, LKML
On Wed, Mar 18, 2020 at 3:34 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Mar 17, 2020 at 11:33 PM Qiujun Huang <hqjagain@gmail.com> wrote:
> >
> > On Wed, Mar 18, 2020 at 1:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Mon, Mar 16, 2020 at 03:18:23PM +0800, Qiujun Huang wrote:
> > > > We should hold idr_mutex for idr_alloc.
> > > >
> > > > Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
> > >
> > > I've not seen the first version of this anywhere in my inbox, not sure
> > > where that got lost.
> > >
> > > Anyway, this seems like a false positive - I'm assuming this was caught
> > > with KCSAN. The commit message really should mention that.
> > >
> > > fill_object_idr creates the idr, which yes is only access later on under
> > > the idr_mutex. But here it's not yet visible to any other thread, and
> > > hence lockless access is safe and correct.
> >
> > Agree that.
>
> Do you know what the recommended annotation for kcsan false positives
> like this should be? Adding kcsan author Marco.
Actually it's not reported by kcsan. I found idr_alloc isn't safe for
parallel modifications,
and I didn't understand it's a exclusive path here. :)
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] drm/lease: fix potential race in fill_object_idr
2020-03-18 7:34 ` Daniel Vetter
2020-03-18 8:09 ` Qiujun Huang
@ 2020-03-18 12:03 ` Marco Elver
1 sibling, 0 replies; 6+ messages in thread
From: Marco Elver @ 2020-03-18 12:03 UTC (permalink / raw)
To: Daniel Vetter
Cc: Qiujun Huang, Maarten Lankhorst, Maxime Ripard, Dave Airlie,
dri-devel, LKML
On Wed, 18 Mar 2020 at 08:34, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Mar 17, 2020 at 11:33 PM Qiujun Huang <hqjagain@gmail.com> wrote:
> >
> > On Wed, Mar 18, 2020 at 1:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Mon, Mar 16, 2020 at 03:18:23PM +0800, Qiujun Huang wrote:
> > > > We should hold idr_mutex for idr_alloc.
> > > >
> > > > Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
> > >
> > > I've not seen the first version of this anywhere in my inbox, not sure
> > > where that got lost.
> > >
> > > Anyway, this seems like a false positive - I'm assuming this was caught
> > > with KCSAN. The commit message really should mention that.
> > >
> > > fill_object_idr creates the idr, which yes is only access later on under
> > > the idr_mutex. But here it's not yet visible to any other thread, and
> > > hence lockless access is safe and correct.
> >
> > Agree that.
>
> Do you know what the recommended annotation for kcsan false positives
> like this should be? Adding kcsan author Marco.
AFAIK KCSAN has not reported this, so I think there is nothing to do here.
Thanks,
-- Marco
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-18 12:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-16 7:18 [PATCH RESEND] drm/lease: fix potential race in fill_object_idr Qiujun Huang
2020-03-17 17:02 ` Daniel Vetter
2020-03-17 22:33 ` Qiujun Huang
2020-03-18 7:34 ` Daniel Vetter
2020-03-18 8:09 ` Qiujun Huang
2020-03-18 12:03 ` Marco Elver
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).