https://bugs.freedesktop.org/show_bug.cgi?id=100077
Bug ID: 100077 Summary: libdrm atomic_add_unless() may reverse return value meaning Product: DRI Version: unspecified Hardware: x86-64 (AMD64) OS: BSD (Others) Status: NEW Severity: normal Priority: medium Component: libdrm Assignee: dri-devel@lists.freedesktop.org Reporter: davshao@gmail.com
atomic_add_unless() in libdrm xf86atomic.h may reverse the meaning of its return value.
Linux kernel documentation seems to indicate something like: "Returns non-zero if @v was not @u, and zero otherwise."
A simple inverting the meaning of libdrm's return value allowed glxgears to properly function for a hacked version of pkgsrc on DragonFly 4.7-DEVELOPMENT on an Intel IvyBridge integrated graphics machine. glxgears was already properly functioning on the same machine for NetBSD current, NetBSD using its own atomic operations declared in its sys/atomic.h header.
A one line (character) fix is of the form:
--- xf86atomic.h.orig 2015-09-22 04:34:51.000000000 +0000 +++ xf86atomic.h @@ -111,7 +111,7 @@ static inline int atomic_add_unless(atom c = atomic_read(v); while (c != unless && (old = atomic_cmpxchg(v, c, c + add)) != c) c = old; - return c == unless; + return c != unless; }
According to the kernel documentation: Returns non-zero if @v was not @u, and zero otherwise.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100077 Fixes: 63fc571863aa64683400 ("atomic: add atomic_add_unless()") Signed-off-by: David Shao davshao@gmail.com Reviewed-by: Eric Engestrom eric@engestrom.ch Signed-off-by: Eric Engestrom eric@engestrom.ch --- xf86atomic.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xf86atomic.h b/xf86atomic.h index 922b37da..ecb1ba86 100644 --- a/xf86atomic.h +++ b/xf86atomic.h @@ -111,7 +111,7 @@ static inline int atomic_add_unless(atomic_t *v, int add, int unless) c = atomic_read(v); while (c != unless && (old = atomic_cmpxchg(v, c, c + add)) != c) c = old; - return c == unless; + return c != unless; }
#endif
On Thursday, 2017-03-16 00:08:59 +0000, Eric Engestrom wrote:
According to the kernel documentation: Returns non-zero if @v was not @u, and zero otherwise.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100077 Fixes: 63fc571863aa64683400 ("atomic: add atomic_add_unless()") Signed-off-by: David Shao davshao@gmail.com Reviewed-by: Eric Engestrom eric@engestrom.ch Signed-off-by: Eric Engestrom eric@engestrom.ch
That last s-o-b was added automatically, by mistake. I'll remove it when pushing this.
This patch seems trivial to verify, can someone confirm this is right?
/me is about ~94% sure, but doesn't know much about atomic ops and their return value convention.
I checked the kernel and it does what libdrm does after this patch, so either one should be fixed; I'm assuming kernel is right and libdrm needs fixing, hence this patch.
Cheers, Eric
xf86atomic.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xf86atomic.h b/xf86atomic.h index 922b37da..ecb1ba86 100644 --- a/xf86atomic.h +++ b/xf86atomic.h @@ -111,7 +111,7 @@ static inline int atomic_add_unless(atomic_t *v, int add, int unless) c = atomic_read(v); while (c != unless && (old = atomic_cmpxchg(v, c, c + add)) != c) c = old;
- return c == unless;
- return c != unless;
}
#endif
Cheers, Eric
[+ Lionel]
On 16 March 2017 at 16:46, Eric Engestrom eric.engestrom@imgtec.com wrote:
On Thursday, 2017-03-16 00:08:59 +0000, Eric Engestrom wrote:
According to the kernel documentation: Returns non-zero if @v was not @u, and zero otherwise.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100077 Fixes: 63fc571863aa64683400 ("atomic: add atomic_add_unless()") Signed-off-by: David Shao davshao@gmail.com Reviewed-by: Eric Engestrom eric@engestrom.ch Signed-off-by: Eric Engestrom eric@engestrom.ch
That last s-o-b was added automatically, by mistake. I'll remove it when pushing this.
This patch seems trivial to verify, can someone confirm this is right?
/me is about ~94% sure, but doesn't know much about atomic ops and their return value convention.
I checked the kernel and it does what libdrm does after this patch, so either one should be fixed; I'm assuming kernel is right and libdrm needs fixing, hence this patch.
Changing the function matches the documentation, yet it doesn't [seem to] align with how it's used. I'm fairly sure that this will break libdrm_intel.
Lionel, can you take a look at this patch please ? https://patchwork.freedesktop.org/patch/144252/
Emil
https://bugs.freedesktop.org/show_bug.cgi?id=100077
GitLab Migration User gitlab-migration@fdo.invalid changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution|--- |MOVED
--- Comment #1 from GitLab Migration User gitlab-migration@fdo.invalid --- -- GitLab Migration Automatic Message --
This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.
You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/drm/issues/17.
dri-devel@lists.freedesktop.org
-
bugzilla-daemon＠freedesktop.org -
Emil Velikov -
Eric Engestrom -
Eric Engestrom