Re: kdump futex fix

[Philip Guenther]

On Sat, 4 Apr 2020, Martin Pieuchot wrote:
> On 03/04/20(Fri) 19:26, Philip Guenther wrote:
> > On Fri, 3 Apr 2020, Martin Pieuchot wrote:
> > > Depending on the operation requested futex(2) might return the number of 
> > > woken threads or an error.  That means the following...
> > > 
> > > mpv  CALL  
> > > futex(0xa58935899b0,0x82,1,0,0)
> > > mpv  RET   futex -1 errno 1 Operation not permitted
> > > 
> > > ...is not an error but it indicates that 1 thread has been awoken.
> > > 
> > > Diff below would have saved me quite some time looking in the wrong 
> > > direction.
> > > 
> > > ok?
> > 
> > Isn't that just a complicated way to get the same effect as deleting the 
> > "case SYS_futex:" line shortly above there?
> 
> Thanks!  Updated below :)

ok guenther@

Re: kdump futex fix

[Paul Irofti]

> The real problem is that futex(2) is actually 3 different syscalls wrapped 
> into one.  It was split into three then kdump could properly report 
> futex_wake(2) and futex_requeue(2) as returning a count, while 
> futex_wait(2) returns an errno.  The existing 'switch' in sys_futex() 
> would just move to userspace's futex(3), provided for linux compat.

I have such a diff from half a year ago. Let me get it back in shape and
I'll send it back here.

Re: kdump futex fix

[Martin Pieuchot]

On 03/04/20(Fri) 19:26, Philip Guenther wrote:
> On Fri, 3 Apr 2020, Martin Pieuchot wrote:
> > Depending on the operation requested futex(2) might return the number of 
> > woken threads or an error.  That means the following...
> > 
> > mpv  CALL  
> > futex(0xa58935899b0,0x82,1,0,0)
> > mpv  RET   futex -1 errno 1 Operation not permitted
> > 
> > ...is not an error but it indicates that 1 thread has been awoken.
> > 
> > Diff below would have saved me quite some time looking in the wrong 
> > direction.
> > 
> > ok?
> 
> Isn't that just a complicated way to get the same effect as deleting the 
> "case SYS_futex:" line shortly above there?

Thanks!  Updated below :)

> The real problem is that futex(2) is actually 3 different syscalls wrapped 
> into one.  It was split into three then kdump could properly report 
> futex_wake(2) and futex_requeue(2) as returning a count, while 
> futex_wait(2) returns an errno.  The existing 'switch' in sys_futex() 
> would just move to userspace's futex(3), provided for linux compat.
> 
> (And our syscalls would have proper prototypes and futex_wait() could take 
> a clockid_t, and)

I already heard this song many times by various people.  I don't mind.
But until now it has just been an idea :)

Index: kdump.c
===
RCS file: /cvs/src/usr.bin/kdump/kdump.c,v
retrieving revision 1.141
diff -u -p -r1.141 kdump.c
--- kdump.c 18 Jan 2020 23:56:31 -  1.141
+++ kdump.c 4 Apr 2020 07:06:20 -
@@ -1128,7 +1128,6 @@ doerr:
/* syscalls that return errno values */
case SYS_getlogin_r:
case SYS___thrsleep:
-   case SYS_futex:
if ((error = ret) != 0)
goto doerr;
/* FALLTHROUGH */

Re: kdump futex fix

[Philip Guenther]

On Fri, 3 Apr 2020, Martin Pieuchot wrote:
> Depending on the operation requested futex(2) might return the number of 
> woken threads or an error.  That means the following...
> 
> mpv  CALL  futex(0xa58935899b0,0x82,1,0,0)
> mpv  RET   futex -1 errno 1 Operation not permitted
> 
> ...is not an error but it indicates that 1 thread has been awoken.
> 
> Diff below would have saved me quite some time looking in the wrong 
> direction.
> 
> ok?

Isn't that just a complicated way to get the same effect as deleting the 
"case SYS_futex:" line shortly above there?

The real problem is that futex(2) is actually 3 different syscalls wrapped 
into one.  It was split into three then kdump could properly report 
futex_wake(2) and futex_requeue(2) as returning a count, while 
futex_wait(2) returns an errno.  The existing 'switch' in sys_futex() 
would just move to userspace's futex(3), provided for linux compat.

(And our syscalls would have proper prototypes and futex_wait() could take 
a clockid_t, and)

kdump futex fix

[Martin Pieuchot]

Depending on the operation requested futex(2) might return the number of
woken threads or an error.  That means the following...

mpv  CALL  futex(0xa58935899b0,0x82,1,0,0)
mpv  RET   futex -1 errno 1 Operation not permitted

...is not an error but it indicates that 1 thread has been awoken.

Diff below would have saved me quite some time looking in the wrong
direction.

ok?

Index: kdump.c
===
RCS file: /cvs/src/usr.bin/kdump/kdump.c,v
retrieving revision 1.141
diff -u -p -r1.141 kdump.c
--- kdump.c 18 Jan 2020 23:56:31 -  1.141
+++ kdump.c 3 Apr 2020 17:27:36 -
@@ -1147,7 +1147,9 @@ doerr:
(void)printf("RESTART");
else if (error == EJUSTRETURN)
(void)printf("JUSTRETURN");
-   else {
+   else if (code == SYS_futex) {
+   (void)printf("%d", error);
+   } else {
(void)printf("-1 errno %d", error);
if (fancy)
(void)printf(" %s", strerror(error));