Re: pledge idea
On Thu, Oct 29, 2015 at 06:39:58PM +0100, Peter J. Philipp wrote: > Hi Reyk, > > deraadt already told me there was a patch for this already. Yes it > would be more cycles for stdio I see that. > > Thanks for your effort in making me see this. > > -peter > > > $ time obj/sleep 0.01 > > 0m00.01s real 0m00.00s user 0m00.01s system > > > > Are you trying to solve an issue? Hi, I just want to revisit this because I couldn't believe it. I turned on system accounting and rebooted my test box. Here is what I found the following programs were run this many times: 23 sh 10 ntpd 9 smtpd 7 domainname 6 id 6 getty 6 getcap 6 basename I'm gonna stop here. Some of these programs were not pledged yet in my sources (-current from last week). So I did the tedious work of adding up the cycles of pledge/strcmp on sh binary and then gave the bsearch idea a guessed average of 6 rounds per lookup. What I came up with was that pledge/strcmp has 120 cycles where bsearch had 60 cycles. Multiplied by 23 times would give pledge/strcmp 2760 cycles and bsearch 1380 cycles. /bin/sh is probably always going to be the most used in any system, well it is at startup. Another comparison was for getty: pledge/strcmp 420 cycles bsearch = 216 cycles getcap had stdio and rpath which is 3 cycles, here it wins against bsearch which has 12 cycles. I do understand that most little programs such as basename have only stdio in pledge and thus will save cycles but when you average it all out by occurences there might be a case for using bsearch? I know it was just halloween but I'm still creeped out by this. -peter
Re: pledge idea
On Thu, Oct 29, 2015 at 04:32:25PM +, Peter J. Philipp wrote:
> Hi deraadt,
>
> I know you know I don't code well, but in order to show you what's on my
> mind I had to write code, I took the bsearch() from the ieee80211 code, so
> perhaps there is a better way (like always) perhaps to unify the function
> between these two areas.
>
> The reason I did this is to save on cpu cycles when you look at X amount
> of processes all pledging...one time'd process won't show much difference.
>
I'm not deraadt, but -
smart but have you considered that this is not worth the effort?
pledge() is only called once or twice during a process' lifetime -
start, pledge, run - the linear search on such a short list is still
relatively fast, and the entries are even sorted in the order of
relevance. By convention "stdio" always comes first. So what is the
impact without your diff of pledge in src/bin/sleep/sleep.c:
if (pledge("stdio", NULL) == -1)
err(1, "pledge");
$ time obj/sleep 0.01
0m00.01s real 0m00.00s user 0m00.01s system
Are you trying to solve an issue?
Reyk
> below's diff:
>
> -peter
>
>
> ? pledge.diff
> Index: kern/kern_pledge.c
> ===
> RCS file: /cvs/src/sys/kern/kern_pledge.c,v
> retrieving revision 1.90
> diff -u -p -u -r1.90 kern_pledge.c
> --- kern/kern_pledge.c28 Oct 2015 17:38:52 - 1.90
> +++ kern/kern_pledge.c29 Oct 2015 16:23:04 -
> @@ -58,6 +58,32 @@
>
> int canonpath(const char *input, char *buf, size_t bufsize);
> int substrcmp(const char *p1, size_t s1, const char *p2, size_t s2);
> +int pledge_cmp(const void *pi, const void *ph);
> +
> +/* taken from net80211/ieee80211_regdomain.c */
> +const void *pledge_bsearch(const void *, const void *, size_t, size_t,
> +int (*)(const void *, const void *));
> +
> +const void *
> +pledge_bsearch(const void *key, const void *base0, size_t nmemb, size_t size,
> +int (*compar)(const void *, const void *))
> +{
> +const char *base = base0;
> +int lim, cmp;
> +const void *p;
> +
> +for (lim = nmemb; lim != 0; lim >>= 1) {
> +p = base + (lim >> 1) * size;
> +cmp = (*compar)(key, p);
> +if (cmp == 0)
> +return ((const void *)p);
> +if (cmp > 0) { /* key > p: move right */
> +base = (const char *)p + size;
> +lim--;
> +} /* else move left */
> +}
> +return (NULL);
> +}
>
> const u_int pledge_syscalls[SYS_MAXSYSCALL] = {
> [SYS_exit] = 0x,
> @@ -256,40 +282,42 @@ const u_int pledge_syscalls[SYS_MAXSYSCA
> [SYS_swapctl] = PLEDGE_VMINFO, /* XXX should limit to "get" operations
> */
> };
>
> -static const struct {
> +/* MUST be sorted! */
> +static const struct PR {
> char *name;
> int flags;
> } pledgereq[] = {
> - { "stdio", PLEDGE_STDIO },
> - { "rpath", PLEDGE_RPATH },
> - { "wpath", PLEDGE_WPATH },
> - { "tmppath",PLEDGE_TMPPATH },
> - { "inet", PLEDGE_INET },
> - { "unix", PLEDGE_UNIX },
> + { "abort", 0 },/* XXX reserve for later */
> + { "cpath", PLEDGE_CPATH },
> { "dns",PLEDGE_DNS },
> + { "exec", PLEDGE_EXEC },
> + { "fattr", PLEDGE_FATTR },
> + { "flock", PLEDGE_FLOCK },
> { "getpw", PLEDGE_GETPW },
> - { "sendfd", PLEDGE_SENDFD },
> - { "recvfd", PLEDGE_RECVFD },
> - { "ioctl", PLEDGE_IOCTL },
> { "id", PLEDGE_ID },
> - { "route", PLEDGE_ROUTE },
> + { "inet", PLEDGE_INET },
> + { "ioctl", PLEDGE_IOCTL },
> { "mcast", PLEDGE_MCAST },
> - { "tty",PLEDGE_TTY },
> { "proc", PLEDGE_PROC },
> - { "exec", PLEDGE_EXEC },
> - { "cpath", PLEDGE_CPATH },
> - { "fattr", PLEDGE_FATTR },
> { "prot_exec", PLEDGE_PROTEXEC },
> - { "flock", PLEDGE_FLOCK },
> { "ps", PLEDGE_PS },
> - { "vminfo", PLEDGE_VMINFO },
> + { "recvfd", PLEDGE_RECVFD },
> + { "route", PLEDGE_ROUTE },
> + { "rpath", PLEDGE_RPATH },
> + { "sendfd", PLEDGE_SENDFD },
> { "settime",PLEDGE_SETTIME },
> - { "abort", 0 },/* XXX reserve for later */
> + { "stdio", PLEDGE_STDIO },
> + { "tmppath",PLEDGE_TMPPATH },
> + { "tty",PLEDGE_TTY },
> + { "unix", PLEDGE_UNIX },
> + { "vminfo", PLEDGE_VMINFO },
pledge idea
Hi deraadt,
I know you know I don't code well, but in order to show you what's on my
mind I had to write code, I took the bsearch() from the ieee80211 code, so
perhaps there is a better way (like always) perhaps to unify the function
between these two areas.
The reason I did this is to save on cpu cycles when you look at X amount
of processes all pledging...one time'd process won't show much difference.
below's diff:
-peter
? pledge.diff
Index: kern/kern_pledge.c
===
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.90
diff -u -p -u -r1.90 kern_pledge.c
--- kern/kern_pledge.c 28 Oct 2015 17:38:52 - 1.90
+++ kern/kern_pledge.c 29 Oct 2015 16:23:04 -
@@ -58,6 +58,32 @@
int canonpath(const char *input, char *buf, size_t bufsize);
int substrcmp(const char *p1, size_t s1, const char *p2, size_t s2);
+int pledge_cmp(const void *pi, const void *ph);
+
+/* taken from net80211/ieee80211_regdomain.c */
+const void *pledge_bsearch(const void *, const void *, size_t, size_t,
+int (*)(const void *, const void *));
+
+const void *
+pledge_bsearch(const void *key, const void *base0, size_t nmemb, size_t size,
+int (*compar)(const void *, const void *))
+{
+const char *base = base0;
+int lim, cmp;
+const void *p;
+
+for (lim = nmemb; lim != 0; lim >>= 1) {
+p = base + (lim >> 1) * size;
+cmp = (*compar)(key, p);
+if (cmp == 0)
+return ((const void *)p);
+if (cmp > 0) { /* key > p: move right */
+base = (const char *)p + size;
+lim--;
+} /* else move left */
+}
+return (NULL);
+}
const u_int pledge_syscalls[SYS_MAXSYSCALL] = {
[SYS_exit] = 0x,
@@ -256,40 +282,42 @@ const u_int pledge_syscalls[SYS_MAXSYSCA
[SYS_swapctl] = PLEDGE_VMINFO, /* XXX should limit to "get" operations
*/
};
-static const struct {
+/* MUST be sorted! */
+static const struct PR {
char *name;
int flags;
} pledgereq[] = {
- { "stdio", PLEDGE_STDIO },
- { "rpath", PLEDGE_RPATH },
- { "wpath", PLEDGE_WPATH },
- { "tmppath",PLEDGE_TMPPATH },
- { "inet", PLEDGE_INET },
- { "unix", PLEDGE_UNIX },
+ { "abort", 0 },/* XXX reserve for later */
+ { "cpath", PLEDGE_CPATH },
{ "dns",PLEDGE_DNS },
+ { "exec", PLEDGE_EXEC },
+ { "fattr", PLEDGE_FATTR },
+ { "flock", PLEDGE_FLOCK },
{ "getpw", PLEDGE_GETPW },
- { "sendfd", PLEDGE_SENDFD },
- { "recvfd", PLEDGE_RECVFD },
- { "ioctl", PLEDGE_IOCTL },
{ "id", PLEDGE_ID },
- { "route", PLEDGE_ROUTE },
+ { "inet", PLEDGE_INET },
+ { "ioctl", PLEDGE_IOCTL },
{ "mcast", PLEDGE_MCAST },
- { "tty",PLEDGE_TTY },
{ "proc", PLEDGE_PROC },
- { "exec", PLEDGE_EXEC },
- { "cpath", PLEDGE_CPATH },
- { "fattr", PLEDGE_FATTR },
{ "prot_exec", PLEDGE_PROTEXEC },
- { "flock", PLEDGE_FLOCK },
{ "ps", PLEDGE_PS },
- { "vminfo", PLEDGE_VMINFO },
+ { "recvfd", PLEDGE_RECVFD },
+ { "route", PLEDGE_ROUTE },
+ { "rpath", PLEDGE_RPATH },
+ { "sendfd", PLEDGE_SENDFD },
{ "settime",PLEDGE_SETTIME },
- { "abort", 0 },/* XXX reserve for later */
+ { "stdio", PLEDGE_STDIO },
+ { "tmppath",PLEDGE_TMPPATH },
+ { "tty",PLEDGE_TTY },
+ { "unix", PLEDGE_UNIX },
+ { "vminfo", PLEDGE_VMINFO },
+ { "wpath", PLEDGE_WPATH },
};
int
sys_pledge(struct proc *p, void *v, register_t *retval)
{
+ struct PR *pr = NULL;
struct sys_pledge_args /* {
syscallarg(const char *)request;
syscallarg(const char **)paths;
@@ -300,7 +328,7 @@ sys_pledge(struct proc *p, void *v, regi
if (SCARG(uap, request)) {
size_t rbuflen;
char *rbuf, *rp, *pn;
- int f, i;
+ int f;
rbuf = malloc(MAXPATHLEN, M_TEMP, M_WAITOK);
error = copyinstr(SCARG(uap, request), rbuf, MAXPATHLEN,
@@ -321,16 +349,15 @@ sys_pledge(struct proc *p, void *v, regi
*pn++ = '\0';
}
- for (f = i = 0; i <
Re: pledge idea
On 10/29/15 18:51, Reyk Floeter wrote:
> On Thu, Oct 29, 2015 at 04:32:25PM +, Peter J. Philipp wrote:
>> Hi deraadt,
>>
>> I know you know I don't code well, but in order to show you what's on my
>> mind I had to write code, I took the bsearch() from the ieee80211 code, so
>> perhaps there is a better way (like always) perhaps to unify the function
>> between these two areas.
>>
>> The reason I did this is to save on cpu cycles when you look at X amount
>> of processes all pledging...one time'd process won't show much difference.
>>
> I'm not deraadt, but -
>
> smart but have you considered that this is not worth the effort?
> pledge() is only called once or twice during a process' lifetime -
> start, pledge, run - the linear search on such a short list is still
> relatively fast, and the entries are even sorted in the order of
> relevance. By convention "stdio" always comes first. So what is the
> impact without your diff of pledge in src/bin/sleep/sleep.c:
>
> if (pledge("stdio", NULL) == -1)
> err(1, "pledge");
Hi Reyk,
deraadt already told me there was a patch for this already. Yes it
would be more cycles for stdio I see that.
Thanks for your effort in making me see this.
-peter
> $ time obj/sleep 0.01
> 0m00.01s real 0m00.00s user 0m00.01s system
>
> Are you trying to solve an issue?
>
> Reyk
>
>> below's diff:
>>
>> -peter
>>
>>
>> ? pledge.diff
>> Index: kern/kern_pledge.c
>> ===
>> RCS file: /cvs/src/sys/kern/kern_pledge.c,v
>> retrieving revision 1.90
>> diff -u -p -u -r1.90 kern_pledge.c
>> --- kern/kern_pledge.c 28 Oct 2015 17:38:52 - 1.90
>> +++ kern/kern_pledge.c 29 Oct 2015 16:23:04 -
>> @@ -58,6 +58,32 @@
>>
>> int canonpath(const char *input, char *buf, size_t bufsize);
>> int substrcmp(const char *p1, size_t s1, const char *p2, size_t s2);
>> +int pledge_cmp(const void *pi, const void *ph);
>> +
>> +/* taken from net80211/ieee80211_regdomain.c */
>> +const void *pledge_bsearch(const void *, const void *, size_t, size_t,
>> +int (*)(const void *, const void *));
>> +
>> +const void *
>> +pledge_bsearch(const void *key, const void *base0, size_t nmemb, size_t
>> size,
>> +int (*compar)(const void *, const void *))
>> +{
>> +const char *base = base0;
>> +int lim, cmp;
>> +const void *p;
>> +
>> +for (lim = nmemb; lim != 0; lim >>= 1) {
>> +p = base + (lim >> 1) * size;
>> +cmp = (*compar)(key, p);
>> +if (cmp == 0)
>> +return ((const void *)p);
>> +if (cmp > 0) { /* key > p: move right */
>> +base = (const char *)p + size;
>> +lim--;
>> +} /* else move left */
>> +}
>> +return (NULL);
>> +}
>>
>> const u_int pledge_syscalls[SYS_MAXSYSCALL] = {
>> [SYS_exit] = 0x,
>> @@ -256,40 +282,42 @@ const u_int pledge_syscalls[SYS_MAXSYSCA
>> [SYS_swapctl] = PLEDGE_VMINFO, /* XXX should limit to "get" operations
>> */
>> };
>>
>> -static const struct {
>> +/* MUST be sorted! */
>> +static const struct PR {
>> char *name;
>> int flags;
>> } pledgereq[] = {
>> -{ "stdio", PLEDGE_STDIO },
>> -{ "rpath", PLEDGE_RPATH },
>> -{ "wpath", PLEDGE_WPATH },
>> -{ "tmppath",PLEDGE_TMPPATH },
>> -{ "inet", PLEDGE_INET },
>> -{ "unix", PLEDGE_UNIX },
>> +{ "abort", 0 },/* XXX reserve for later */
>> +{ "cpath", PLEDGE_CPATH },
>> { "dns",PLEDGE_DNS },
>> +{ "exec", PLEDGE_EXEC },
>> +{ "fattr", PLEDGE_FATTR },
>> +{ "flock", PLEDGE_FLOCK },
>> { "getpw", PLEDGE_GETPW },
>> -{ "sendfd", PLEDGE_SENDFD },
>> -{ "recvfd", PLEDGE_RECVFD },
>> -{ "ioctl", PLEDGE_IOCTL },
>> { "id", PLEDGE_ID },
>> -{ "route", PLEDGE_ROUTE },
>> +{ "inet", PLEDGE_INET },
>> +{ "ioctl", PLEDGE_IOCTL },
>> { "mcast", PLEDGE_MCAST },
>> -{ "tty",PLEDGE_TTY },
>> { "proc", PLEDGE_PROC },
>> -{ "exec", PLEDGE_EXEC },
>> -{ "cpath", PLEDGE_CPATH },
>> -{ "fattr", PLEDGE_FATTR },
>> { "prot_exec", PLEDGE_PROTEXEC },
>> -{ "flock", PLEDGE_FLOCK },
>> { "ps", PLEDGE_PS },
>> -{ "vminfo", PLEDGE_VMINFO },
>> +{ "recvfd", PLEDGE_RECVFD },
>> +{ "route", PLEDGE_ROUTE },
>> +{ "rpath", PLEDGE_RPATH },
>> +{ "sendfd", PLEDGE_SENDFD },
>> { "settime",PLEDGE_SETTIME },
