Re: [CALL FOR TESTERS] New system call: abort2()
On Mon, Dec 19, 2005 at 02:24:04PM -0500, John Baldwin wrote: On Friday 16 December 2005 05:19 pm, Wojciech A. Koszek wrote: On Fri, Dec 16, 2005 at 11:14:12AM -0500, John Baldwin wrote: On Friday 16 December 2005 04:10 am, Peter Jeremy wrote: On Thu, 2005-Dec-15 22:37:45 +, Wojciech A. Koszek wrote: abort2(const char *why, int nargs, void **args); why is reason of program abort, nargs is number of arguments passed in args. Both why and args (with %p format) will be printed via log(9). Sample output: [..] pid 3004 abort2 abort2: ABORT2 arg0:0x62612f2e pid 3019 abort2 abort2: invalid argument [..] I don't believe the following code is correct. uap-args is a userspace pointer so uap-args[i] is dereferencing a userspace argument in kernelspace. + arg = uargs[i] = (void *) fuword(uap-args[i]); I think it should be fuword(uap-args + i); I don't see the point of the following test. arg is printed using %p and never de-referenced so there's no reason it can't be NULL. I would see that a legitimate use of abort2() is when the application detects that a pointer is unexpectedly NULL. Aborting on -1 is less clear - if fuword() fails, it will return -1 but, equally, a faulty user application may have left -1 in a pointer. (Note that mmap(2) returns -1 on error so it's not inconceivable that a pointer could contain -1). + /* Prevent from faults in user-space */ + if (arg == NULL || arg == (void *)-1) { + error = EINVAL; + break; + } Taking the above into account, I believe the code should be: + if (uap-args == NULL) + break; + error = copyin(uap-args, uargs, uap-nargs * sizeof (void *)); + if (error != 0) + break; Agreed. Also, copyinstr() can provide a better interface for copying the why string in. Also, the PROC LOCK isn't needed for reading the static p_pid and p_comm fields of struct proc. Also, I second the other comments of do { } while(0) vs goto. Many existing syscalls use 'goto out;' for error handling, and I think that is one of the very few cases when goto is useful and not harmful. Thanks for the suggestions and comments! My question is related with copying string from user-space: the only difference I can see between those functions (other than operating of strings/sbufs) is that sbuf_copyin() looses 'done' [1]. Since current abort2() makes use of sbuf(9), I'll have to have additional buffer just for string copying and than copy it to sbuf. Would you prefer this solution or complete migration from sbufs to strl..()? [1] Couldn't sbuf_copyin() simply return 'done' from copyinstr() embedded in it, since it already returns -1 on failure? This function is used in two places, which make no use of return value. That sounds good to me (fixing sbuf_copyin()). I got no response from people for whom changing this function could be a problem. My patch is here: http://freebsd.czest.pl/dunstan/FreeBSD/sbuf_copyin.0.patch -- * Wojciech A. Koszek [EMAIL PROTECTED] ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: [CALL FOR TESTERS] New system call: abort2()
On Fri, Dec 16, 2005 at 11:14:12AM -0500, John Baldwin wrote: On Friday 16 December 2005 04:10 am, Peter Jeremy wrote: On Thu, 2005-Dec-15 22:37:45 +, Wojciech A. Koszek wrote: abort2(const char *why, int nargs, void **args); why is reason of program abort, nargs is number of arguments passed in args. Both why and args (with %p format) will be printed via log(9). Sample output: [..] pid 3004 abort2 abort2: ABORT2 arg0:0x62612f2e pid 3019 abort2 abort2: invalid argument [..] [..] Agreed. Also, copyinstr() can provide a better interface for copying the why string in. Also, the PROC LOCK isn't needed for reading the static p_pid and p_comm fields of struct proc. Also, I second the other comments of do { } while(0) vs goto. Many existing syscalls use 'goto out;' for error handling, and I think that is one of the very few cases when goto is useful and not harmful. Updated patch is here: http://freebsd.czest.pl/dunstan/FreeBSD/abort2/abort2.3.patch If I have to change something, let me know. Once again -- comments are welcome. -- * Wojciech A. Koszek [EMAIL PROTECTED] ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: [CALL FOR TESTERS] New system call: abort2()
On Friday 16 December 2005 05:19 pm, Wojciech A. Koszek wrote: On Fri, Dec 16, 2005 at 11:14:12AM -0500, John Baldwin wrote: On Friday 16 December 2005 04:10 am, Peter Jeremy wrote: On Thu, 2005-Dec-15 22:37:45 +, Wojciech A. Koszek wrote: abort2(const char *why, int nargs, void **args); why is reason of program abort, nargs is number of arguments passed in args. Both why and args (with %p format) will be printed via log(9). Sample output: [..] pid 3004 abort2 abort2: ABORT2 arg0:0x62612f2e pid 3019 abort2 abort2: invalid argument [..] I don't believe the following code is correct. uap-args is a userspace pointer so uap-args[i] is dereferencing a userspace argument in kernelspace. + arg = uargs[i] = (void *) fuword(uap-args[i]); I think it should be fuword(uap-args + i); I don't see the point of the following test. arg is printed using %p and never de-referenced so there's no reason it can't be NULL. I would see that a legitimate use of abort2() is when the application detects that a pointer is unexpectedly NULL. Aborting on -1 is less clear - if fuword() fails, it will return -1 but, equally, a faulty user application may have left -1 in a pointer. (Note that mmap(2) returns -1 on error so it's not inconceivable that a pointer could contain -1). + /* Prevent from faults in user-space */ + if (arg == NULL || arg == (void *)-1) { + error = EINVAL; + break; + } Taking the above into account, I believe the code should be: + if (uap-args == NULL) + break; + error = copyin(uap-args, uargs, uap-nargs * sizeof (void *)); + if (error != 0) + break; Agreed. Also, copyinstr() can provide a better interface for copying the why string in. Also, the PROC LOCK isn't needed for reading the static p_pid and p_comm fields of struct proc. Also, I second the other comments of do { } while(0) vs goto. Many existing syscalls use 'goto out;' for error handling, and I think that is one of the very few cases when goto is useful and not harmful. Thanks for the suggestions and comments! My question is related with copying string from user-space: the only difference I can see between those functions (other than operating of strings/sbufs) is that sbuf_copyin() looses 'done' [1]. Since current abort2() makes use of sbuf(9), I'll have to have additional buffer just for string copying and than copy it to sbuf. Would you prefer this solution or complete migration from sbufs to strl..()? [1] Couldn't sbuf_copyin() simply return 'done' from copyinstr() embedded in it, since it already returns -1 on failure? This function is used in two places, which make no use of return value. That sounds good to me (fixing sbuf_copyin()). -- John Baldwin [EMAIL PROTECTED]http://www.FreeBSD.org/~jhb/ Power Users Use the Power to Serve = http://www.FreeBSD.org ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: [CALL FOR TESTERS] New system call: abort2()
On Fri, 2005-Dec-16 00:17:14 +0100, Václav Haisman wrote: I would like to comment on FreeBSD style(9) a bit. That is a different bikeshed. If, taking into account the other comments in this thread, you believe that you can justify changes to style(9), please start a new thread - probably in -arch. -- Peter Jeremy ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: [CALL FOR TESTERS] New system call: abort2()
On Thu, 2005-Dec-15 22:37:45 +, Wojciech A. Koszek wrote: abort2(const char *why, int nargs, void **args); why is reason of program abort, nargs is number of arguments passed in args. Both why and args (with %p format) will be printed via log(9). Sample output: [..] pid 3004 abort2 abort2: ABORT2 arg0:0x62612f2e pid 3019 abort2 abort2: invalid argument [..] I don't believe the following code is correct. uap-args is a userspace pointer so uap-args[i] is dereferencing a userspace argument in kernelspace. + arg = uargs[i] = (void *) fuword(uap-args[i]); I think it should be fuword(uap-args + i); I don't see the point of the following test. arg is printed using %p and never de-referenced so there's no reason it can't be NULL. I would see that a legitimate use of abort2() is when the application detects that a pointer is unexpectedly NULL. Aborting on -1 is less clear - if fuword() fails, it will return -1 but, equally, a faulty user application may have left -1 in a pointer. (Note that mmap(2) returns -1 on error so it's not inconceivable that a pointer could contain -1). + /* Prevent from faults in user-space */ + if (arg == NULL || arg == (void *)-1) { + error = EINVAL; + break; + } Taking the above into account, I believe the code should be: + if (uap-args == NULL) + break; + error = copyin(uap-args, uargs, uap-nargs * sizeof (void *)); + if (error != 0) + break; -- Peter Jeremy ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: [CALL FOR TESTERS] New system call: abort2()
On Friday 16 December 2005 04:10 am, Peter Jeremy wrote: On Thu, 2005-Dec-15 22:37:45 +, Wojciech A. Koszek wrote: abort2(const char *why, int nargs, void **args); why is reason of program abort, nargs is number of arguments passed in args. Both why and args (with %p format) will be printed via log(9). Sample output: [..] pid 3004 abort2 abort2: ABORT2 arg0:0x62612f2e pid 3019 abort2 abort2: invalid argument [..] I don't believe the following code is correct. uap-args is a userspace pointer so uap-args[i] is dereferencing a userspace argument in kernelspace. + arg = uargs[i] = (void *) fuword(uap-args[i]); I think it should be fuword(uap-args + i); I don't see the point of the following test. arg is printed using %p and never de-referenced so there's no reason it can't be NULL. I would see that a legitimate use of abort2() is when the application detects that a pointer is unexpectedly NULL. Aborting on -1 is less clear - if fuword() fails, it will return -1 but, equally, a faulty user application may have left -1 in a pointer. (Note that mmap(2) returns -1 on error so it's not inconceivable that a pointer could contain -1). + /* Prevent from faults in user-space */ + if (arg == NULL || arg == (void *)-1) { + error = EINVAL; + break; + } Taking the above into account, I believe the code should be: + if (uap-args == NULL) + break; + error = copyin(uap-args, uargs, uap-nargs * sizeof (void *)); + if (error != 0) + break; Agreed. Also, copyinstr() can provide a better interface for copying the why string in. Also, the PROC LOCK isn't needed for reading the static p_pid and p_comm fields of struct proc. Also, I second the other comments of do { } while(0) vs goto. Many existing syscalls use 'goto out;' for error handling, and I think that is one of the very few cases when goto is useful and not harmful. -- John Baldwin [EMAIL PROTECTED]http://www.FreeBSD.org/~jhb/ Power Users Use the Power to Serve = http://www.FreeBSD.org ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: [CALL FOR TESTERS] New system call: abort2()
On Fri, Dec 16, 2005 at 08:10:57PM +1100, Peter Jeremy wrote: On Thu, 2005-Dec-15 22:37:45 +, Wojciech A. Koszek wrote: abort2(const char *why, int nargs, void **args); why is reason of program abort, nargs is number of arguments passed in args. Both why and args (with %p format) will be printed via log(9). Sample output: [..] pid 3004 abort2 abort2: ABORT2 arg0:0x62612f2e pid 3019 abort2 abort2: invalid argument [..] I don't believe the following code is correct. uap-args is a userspace pointer so uap-args[i] is dereferencing a userspace argument in kernelspace. + arg = uargs[i] = (void *) fuword(uap-args[i]); I think it should be fuword(uap-args + i); I don't see the point of the following test. arg is printed using %p and never de-referenced so there's no reason it can't be NULL. I would see that a legitimate use of abort2() is when the application detects that a pointer is unexpectedly NULL. Aborting on -1 is less clear - if fuword() fails, it will return -1 but, equally, a faulty user application may have left -1 in a pointer. (Note that mmap(2) returns -1 on error so it's not inconceivable that a pointer could contain -1). + /* Prevent from faults in user-space */ + if (arg == NULL || arg == (void *)-1) { + error = EINVAL; + break; + } Taking the above into account, I believe the code should be: + if (uap-args == NULL) + break; + error = copyin(uap-args, uargs, uap-nargs * sizeof (void *)); + if (error != 0) + break; Of course! Thanks for this comment! Updated version should be available soon. -- * Wojciech A. Koszek [EMAIL PROTECTED] ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: [CALL FOR TESTERS] New system call: abort2()
On Fri, Dec 16, 2005 at 11:14:12AM -0500, John Baldwin wrote: On Friday 16 December 2005 04:10 am, Peter Jeremy wrote: On Thu, 2005-Dec-15 22:37:45 +, Wojciech A. Koszek wrote: abort2(const char *why, int nargs, void **args); why is reason of program abort, nargs is number of arguments passed in args. Both why and args (with %p format) will be printed via log(9). Sample output: [..] pid 3004 abort2 abort2: ABORT2 arg0:0x62612f2e pid 3019 abort2 abort2: invalid argument [..] I don't believe the following code is correct. uap-args is a userspace pointer so uap-args[i] is dereferencing a userspace argument in kernelspace. + arg = uargs[i] = (void *) fuword(uap-args[i]); I think it should be fuword(uap-args + i); I don't see the point of the following test. arg is printed using %p and never de-referenced so there's no reason it can't be NULL. I would see that a legitimate use of abort2() is when the application detects that a pointer is unexpectedly NULL. Aborting on -1 is less clear - if fuword() fails, it will return -1 but, equally, a faulty user application may have left -1 in a pointer. (Note that mmap(2) returns -1 on error so it's not inconceivable that a pointer could contain -1). + /* Prevent from faults in user-space */ + if (arg == NULL || arg == (void *)-1) { + error = EINVAL; + break; + } Taking the above into account, I believe the code should be: + if (uap-args == NULL) + break; + error = copyin(uap-args, uargs, uap-nargs * sizeof (void *)); + if (error != 0) + break; Agreed. Also, copyinstr() can provide a better interface for copying the why string in. Also, the PROC LOCK isn't needed for reading the static p_pid and p_comm fields of struct proc. Also, I second the other comments of do { } while(0) vs goto. Many existing syscalls use 'goto out;' for error handling, and I think that is one of the very few cases when goto is useful and not harmful. Thanks for the suggestions and comments! My question is related with copying string from user-space: the only difference I can see between those functions (other than operating of strings/sbufs) is that sbuf_copyin() looses 'done' [1]. Since current abort2() makes use of sbuf(9), I'll have to have additional buffer just for string copying and than copy it to sbuf. Would you prefer this solution or complete migration from sbufs to strl..()? [1] Couldn't sbuf_copyin() simply return 'done' from copyinstr() embedded in it, since it already returns -1 on failure? This function is used in two places, which make no use of return value. -- * Wojciech A. Koszek [EMAIL PROTECTED] ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
[CALL FOR TESTERS] New system call: abort2()
(discussed task was picked from Poul-Henning Kamp's TODO list) Hackers, I've implemented abort2() system call. Works just like abort(3), but delivers signal reliably. Here is a prototype: abort2(const char *why, int nargs, void **args); why is reason of program abort, nargs is number of arguments passed in args. Both why and args (with %p format) will be printed via log(9). Sample output: [..] pid 3004 abort2 abort2: ABORT2 arg0:0x62612f2e pid 3019 abort2 abort2: invalid argument [..] I put two versions: one implemented as KLD so that more people can test it without a problem: http://freebsd.czest.pl/dunstan/FreeBSD/abort2/abort2-0.3.tgz ..and also as a patch: http://freebsd.czest.pl/dunstan/FreeBSD/abort2/diff.1.abort2-sycall If I miss some tests, please let me know, especially in user-kernelspace interaction area. (currently I run patched kernel and couldn't get a panic in testing process). Short description - function: abort2(const char *why, int nargs, void **args); ..never returns. If improper arguments are used, instead of SIGABRT, SIGKILL is delivered. By improper arguments I understand: invalid pointers -- why or args or 0 nargs 16. Comments are welcome! -- * Wojciech A. Koszek [EMAIL PROTECTED] ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: [CALL FOR TESTERS] New system call: abort2()
Wojciech A. Koszek wrote: [...] Comments are welcome! As for the patch, the use of do {} while(0) instead of goto looks odd to me. I would like to comment on FreeBSD style(9) a bit. Why does not mention or even encourage C99 style // comments? They are nice when one wants to comment out bigger chunks of code with /**/ comment. On the similar note, the ability to move declarations closer to the point of use in code is IMO nice feature, too. The style(9) doesn't mention this either. Vaclav Haisman signature.asc Description: OpenPGP digital signature
Re: [CALL FOR TESTERS] New system call: abort2()
On Fri, Dec 16, 2005 at 12:17:14AM +0100, Václav Haisman wrote: I would like to comment on FreeBSD style(9) a bit. Why does not mention or even encourage C99 style // comments? They are nice when one wants to comment out bigger chunks of code with /**/ comment. Use #if 0 ... #endif for that. Joerg On the similar note, the ability to move declarations closer to the point of use in code is IMO nice feature, too. The style(9) doesn't mention this either. Because it is not necessarily a good thing. Joerg ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: [CALL FOR TESTERS] New system call: abort2()
I would like to comment on FreeBSD style(9) a bit. Why does not mention or even encourage C99 style // comments? They are nice when one wants to comment out bigger chunks of code with /**/ comment. Too new. /**/ comment out is bogus anyway. #if 0 ... #endif is better. On the similar note, the ability to move declarations closer to the point of use in code is IMO nice feature, too. The style(9) doesn't mention this either. C doesn't allow it, or didn't until recently. That style tends to lead to really gross things too. Functions should be short enough that it doesn't matter. But changing style(9) is hard. We have better things to do with our lives. :-) Warner ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: [CALL FOR TESTERS] New system call: abort2()
In message [EMAIL PROTECTED], Warner Losh writes: On the similar note, the ability to move declarations closer to the point of use in code is IMO nice feature, too. The style(9) doesn't mention this either. C doesn't allow it, or didn't until recently. That style tends to lead to really gross things too. Functions should be short enough that it doesn't matter. Also, it tends to make it harder to judge the amount of stackspace a function uses, something which is not entirely uninteresting in kernel programming. And yes, changing style(9) is just not worth the time it takes. -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: [CALL FOR TESTERS] New system call: abort2()
[EMAIL PROTECTED] wrote: On Fri, Dec 16, 2005 at 12:17:14AM +0100, Václav Haisman wrote: I would like to comment on FreeBSD style(9) a bit. Why does not mention or even encourage C99 style // comments? They are nice when one wants to comment out bigger chunks of code with /**/ comment. Use #if 0 ... #endif for that. Well, that was the only option in time of C90, now we have IMO better facility for this. Also most editors do not mark such code as commented out which might a bit inconvenient. Joerg On the similar note, the ability to move declarations closer to the point of use in code is IMO nice feature, too. The style(9) doesn't mention this either. Because it is not necessarily a good thing. I am of different opinion but I will leave it at that. Joerg Vaclav Haisman signature.asc Description: OpenPGP digital signature
Re: [CALL FOR TESTERS] New system call: abort2()
Poul-Henning Kamp wrote: In message [EMAIL PROTECTED], Warner Losh writes: On the similar note, the ability to move declarations closer to the point of use in code is IMO nice feature, too. The style(9) doesn't mention this either. C doesn't allow it, or didn't until recently. That style tends to lead to really gross things too. Functions should be short enough that it doesn't matter. (I haven't gotten this email yet.) Anything can lead to really gross things if misused. Also, it tends to make it harder to judge the amount of stackspace a function uses, something which is not entirely uninteresting in kernel programming. While it might be harder to get estimate of stack space allocation I suspect it could actually lower the allocation. And yes, changing style(9) is just not worth the time it takes. Vaclav Haisman signature.asc Description: OpenPGP digital signature
Re: [CALL FOR TESTERS] New system call: abort2()
Also, it tends to make it harder to judge the amount of stackspace a function uses, something which is not entirely uninteresting in kernel programming. While it might be harder to get estimate of stack space allocation I suspect it could actually lower the allocation. Maybe, maybe not. Experience has shown that putting things in inner scopes only sometimes reduces stack usage, and then it depends on the compiler, options used, etc. Warner ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]
Re: [CALL FOR TESTERS] New system call: abort2()
On Fri, Dec 16, 2005 at 12:17:14AM +0100, Václav Haisman wrote: Wojciech A. Koszek wrote: [...] Comments are welcome! As for the patch, the use of do {} while(0) instead of goto looks odd to me. This can be changed easily in final version of the patch if needed. I would like to comment on FreeBSD style(9) a bit. Why does not mention or even encourage C99 style // comments? They are nice when one wants to comment out bigger chunks of code with /**/ comment. On the similar note, the ability to move declarations closer to the point of use in code is IMO nice feature, too. The style(9) doesn't mention this either. This creates unnecessary problems: large blocks can duplicate name of the variable so that they overlap, which has happened in the past. Additionally, some files use this kind of declaration, and it makes source hard to read. Also please note that current comments probably needs changing. I'm looking forward to hearing new comments in that field. Thanks, -- * Wojciech A. Koszek [EMAIL PROTECTED] ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to [EMAIL PROTECTED]