* [patch] Modify flag's value in tapset's probe delete_module [not found] <1188780895.5736.ezmlm@sourceware.org> @ 2007-09-03 12:48 ` Zhaolei 2007-09-05 14:46 ` [patch] For memory access error when calling _stp_sockaddr_str with addrlen set to 0 Zhaolei 0 siblings, 1 reply; 8+ messages in thread From: Zhaolei @ 2007-09-03 12:48 UTC (permalink / raw) To: systemtap; +Cc: Zhaolei Hi, everyone In the probe delete_module(syscalls.stp), function _module_flags_str (aux_syscalls.stp) is used for converting the flags to string. But the returned string is different from the man page of delete_module: delete_module's flag values in "man delete_module"(RHEL5,IA64) is O_TRUNC and O_NONBLOCK, but in probe delete_module, flag's value is WAIT and FORCE (From other non-intel-arch?). Is there anyone knows why tapset uses WAIT/FORCE values for delete_module's flag, please tell me(or I will modify it unify with man). And there may be another problem exists in delete_module's probe: O_TRUNC and O_NONBLOCK's value is different between architectures, so I think it is better using defined value for compare with flag's value. (It we use static value as 2048/512, it will get different result across architectures): I write a patch for this problem: and if no objection, I will commit it. Signed-off-by: "Zhaolei" zhaolei@cn.fujitsu.com function _module_flags_str:string(flags:long) --- aux_syscalls.stp.old 2007-08-24 15:35:05.000000000 +0900 +++ aux_syscalls.stp 2007-08-24 15:34:44.000000000 +0900 @@ -1131,11 +1131,19 @@ function _mlockall_flags_str:string(flag %} /* used by sys_delete_module */ -function _module_flags_str(f) { - if(!(f & 2048)) bs="WAIT|" - if(f & 512) bs=bs."FORCE|" - return substr(bs,0,strlen(bs)-1) -} +function _module_flags_str:string(flags:long) +%{ /* pure */ + int len; + long flags = THIS->flags; + char *str = THIS->__retvalue; + if (flags & O_TRUNC) + strlcat(str,"O_TRUNC|", MAXSTRINGLEN); + if (flags & O_NONBLOCK) + strlcat(str,"O_NONBLOCK|", MAXSTRINGLEN); + len = strlen(str); + if (len) + str[strlen(str)-1] = 0; +%} function _sched_policy_str(policy) { if(policy==0) return "SCHED_OTHER" Regards Zhaolei ^ permalink raw reply [flat|nested] 8+ messages in thread
* [patch] For memory access error when calling _stp_sockaddr_str with addrlen set to 0 2007-09-03 12:48 ` [patch] Modify flag's value in tapset's probe delete_module Zhaolei @ 2007-09-05 14:46 ` Zhaolei 2007-09-05 22:49 ` Frank Ch. Eigler 2007-09-10 5:03 ` [patch] For getrusage's argstr in tapset Zhaolei 0 siblings, 2 replies; 8+ messages in thread From: Zhaolei @ 2007-09-05 14:46 UTC (permalink / raw) To: systemtap; +Cc: Zhaolei Hi, everyone I found a memory access error in aux_syscall.stp's _stp_sockaddr_str function. When probing a function which uses _stp_sockaddr_str with addrlen set to 0, Ex: bind(sockfd, my_addr, 0); the calling stack is: probe bind [bind(sockfd, my_addr, 0)] --> _struct_sockaddr_u [copy 0 byte from my_addr in userspace to buf] --> _stp_sockaddr_str [access buf's content in switch (sa->sa_family)] In this case, sa->sa_family will not be initialized. Also, I think the buf's length should be considered when using it. If no objection, I will commit the following patch: Signed-off-by: "Zhaolei" zhaolei@cn.fujitsu.com --- aux_syscalls.stp.old 2007-08-30 14:56:29.000000000 +0900 +++ aux_syscalls.stp 2007-08-30 15:38:40.000000000 +0900 @@ -309,36 +309,31 @@ function _struct_itimerval:string(addr:l void _stp_sockaddr_str(char *str, const int strlen, char *buf, int len) { struct sockaddr *sa = (struct sockaddr *)buf; - switch (sa->sa_family) { - case AF_INET: + if ((sa->sa_family == AF_INET)&&(len == 16)) { struct sockaddr_in *sin = (struct sockaddr_in *)buf; const unsigned char *addr = (unsigned char *)&sin->sin_addr; snprintf(str, strlen, "{AF_INET, %d.%d.%d.%d, %d}", addr[0], addr[1], addr[2], addr[3], ntohs(sin->sin_port)); - break; } - case AF_UNIX: + else if ((sa->sa_family == AF_UNIX)&&(len == 110)) { struct sockaddr_un *sun = (struct sockaddr_un *)buf; snprintf(str, strlen, "{AF_UNIX, %s}", sun->sun_path); - break; } - case AF_NETLINK: + else if ((sa->sa_family == AF_NETLINK)&&(len == 12)) { struct sockaddr_nl *nl = (struct sockaddr_nl *)buf; snprintf(str, strlen, "{AF_NETLINK, pid=%d, groups=%08x}", nl->nl_pid, nl->nl_groups); - break; } - case AF_INET6: + else if ((sa->sa_family == AF_INET6)&&(len == 28)) { // FIXME. Address is probably not correctly displayed struct sockaddr_in6 *sin = (struct sockaddr_in6 *)buf; snprintf(str, strlen, "{AF_INET6, %016llx, %d}", *(long long *)&sin->sin6_addr, ntohs(sin->sin6_port)); - break; } - case AF_PACKET: + else if ((sa->sa_family == AF_PACKET)&&(len == 18)) { /* FIXME. This needs tested */ struct sockaddr_ll *sll = (struct sockaddr_ll *)buf; @@ -351,11 +346,17 @@ void _stp_sockaddr_str(char *str, const (int)sll->sll_protocol, sll->sll_ifindex, (int)sll->sll_hatype, (int)sll->sll_pkttype, (int)sll->sll_halen, *(uint64_t *)sll->sll_addr); #endif - break; } - - default: - snprintf(str, strlen, "{unknown address family %d}", sa->sa_family); + else + { + if (len >= 2) + { + snprintf(str, strlen, "{unknown sockaddr with sa=%d, salen=%d}", sa->sa_family, len); + } + else + { + snprintf(str, strlen, "{unknown sockaddr with salen=%d}", len); + } } } %} Regards Zhaolei ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] For memory access error when calling _stp_sockaddr_str with addrlen set to 0 2007-09-05 14:46 ` [patch] For memory access error when calling _stp_sockaddr_str with addrlen set to 0 Zhaolei @ 2007-09-05 22:49 ` Frank Ch. Eigler 2007-09-10 5:03 ` [patch] For getrusage's argstr in tapset Zhaolei 1 sibling, 0 replies; 8+ messages in thread From: Frank Ch. Eigler @ 2007-09-05 22:49 UTC (permalink / raw) To: Zhaolei; +Cc: systemtap "Zhaolei" <zhaolei@cn.fujitsu.com> writes: > I found a memory access error in aux_syscall.stp's _stp_sockaddr_str > function. Thanks. > [...] Also, I think the buf's length should be considered when > using it. If no objection, I will commit the following patch: Looks good, except that I suggest using sizeof()-based expressions instead of literal numbers for the length comparisons, if possible. - FChE ^ permalink raw reply [flat|nested] 8+ messages in thread
* [patch] For getrusage's argstr in tapset 2007-09-05 14:46 ` [patch] For memory access error when calling _stp_sockaddr_str with addrlen set to 0 Zhaolei 2007-09-05 22:49 ` Frank Ch. Eigler @ 2007-09-10 5:03 ` Zhaolei 2007-09-10 5:21 ` Wenji Huang 1 sibling, 1 reply; 8+ messages in thread From: Zhaolei @ 2007-09-10 5:03 UTC (permalink / raw) To: systemtap; +Cc: Zhaolei Hi, everyone In [tapset->probe getrusage], argstr's "who" argument is printed as "RUSAGE_BOTH" when its value is -2. But "RUSAGE_BOTH" is not used in getrusage syscall due to the following kernel source: asmlinkage long sys_getrusage(int who, struct rusage __user *ru) { if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) return -EINVAL; return getrusage(current, who, ru); } So i want to remove RUSAGE_BOTH from _rusage_who_str. Is there anyone knows why RUSAGE_BOTH is necessary, please tell me. and if no objection, I will commit it. Signed-off-by: "Zhaolei" zhaolei@cn.fujitsu.com --- aux_syscalls.stp.old 2007-09-06 11:51:14.000000000 +0900 +++ aux_syscalls.stp 2007-09-06 11:52:22.000000000 +0900 @@ -1661,7 +1661,6 @@ function _rlimit_resource_str(r) { function _rusage_who_str(w) { if(w==0) return "RUSAGE_SELF" if(w==-1) return "RUSAGE_CHILDREN" - if(w==-2) return "RUSAGE_BOTH" return sprintf("UNKNOWN VALUE: %d", w) } Regards Zhaolei ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] For getrusage's argstr in tapset 2007-09-10 5:03 ` [patch] For getrusage's argstr in tapset Zhaolei @ 2007-09-10 5:21 ` Wenji Huang 2007-09-10 9:28 ` Zhaolei 0 siblings, 1 reply; 8+ messages in thread From: Wenji Huang @ 2007-09-10 5:21 UTC (permalink / raw) To: Zhaolei; +Cc: systemtap Hi Zhaolei, probe getrusage set probe point at the entry of the function. So the arguments may be any value. So I think to keep the entry in _rusage_who_str, just give another mapping in case of some certain value. Of course, if you can ensure that "who" only be RUSAGE_SELF/RUSAGE_CHILDREN when the syscall called. Just remove. Thanks, Wenji Zhaolei wrote: >Hi, everyone > >In [tapset->probe getrusage], argstr's "who" argument is printed as > "RUSAGE_BOTH" when its value is -2. >But "RUSAGE_BOTH" is not used in getrusage syscall due to the following > kernel source: >asmlinkage long sys_getrusage(int who, struct rusage __user *ru) >{ > if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) > return -EINVAL; > return getrusage(current, who, ru); >} > >So i want to remove RUSAGE_BOTH from _rusage_who_str. >Is there anyone knows why RUSAGE_BOTH is necessary, please tell me. >and if no objection, I will commit it. > >Signed-off-by: "Zhaolei" zhaolei@cn.fujitsu.com > >--- aux_syscalls.stp.old 2007-09-06 11:51:14.000000000 +0900 >+++ aux_syscalls.stp 2007-09-06 11:52:22.000000000 +0900 >@@ -1661,7 +1661,6 @@ function _rlimit_resource_str(r) { > function _rusage_who_str(w) { > if(w==0) return "RUSAGE_SELF" > if(w==-1) return "RUSAGE_CHILDREN" >- if(w==-2) return "RUSAGE_BOTH" > return sprintf("UNKNOWN VALUE: %d", w) > } > >Regards >Zhaolei > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] For getrusage's argstr in tapset 2007-09-10 5:21 ` Wenji Huang @ 2007-09-10 9:28 ` Zhaolei 2007-09-10 14:01 ` Wenji Huang 0 siblings, 1 reply; 8+ messages in thread From: Zhaolei @ 2007-09-10 9:28 UTC (permalink / raw) To: Wenji Huang; +Cc: systemtap Hello, Wenji. Thank you for your suggestion. I investigated RUSAGE_BOTH and got the following result: 1: _rusage_who_str is only used for sys_getrusage(and compat_sys_getrusage) in current tapset, and there is no other syscall which needs to call _rusage_who_str. 2: RUSAGE_BOTH is only used in kernel's internal function getrusage, but there is no syscall using this function with RUSAGE_BOTH. So I think RUSAGE_BOTH is not necessary in tracing syscall, but it may be needed when tracing kernel function as getrusage. It is good idea disable RUSAGE_BOTH only in sys_getrusage (and compat_sys_getrusage). Regards Zhaolei ----- Original Message ----- From: "Wenji Huang" <wenji.huang@oracle.com> To: "Zhaolei" <zhaolei@cn.fujitsu.com> Cc: <systemtap@sourceware.org> Sent: Monday, September 10, 2007 11:44 AM Subject: Re: [patch] For getrusage's argstr in tapset > Hi Zhaolei, > > probe getrusage set probe point at the entry of the function. So the > arguments may be any value. > > So I think to keep the entry in _rusage_who_str, just give another > mapping in case of some certain value. > > Of course, if you can ensure that "who" only be > RUSAGE_SELF/RUSAGE_CHILDREN when the syscall called. Just remove. > > Thanks, > Wenji > > > Zhaolei wrote: > > >Hi, everyone > > > >In [tapset->probe getrusage], argstr's "who" argument is printed as > > "RUSAGE_BOTH" when its value is -2. > >But "RUSAGE_BOTH" is not used in getrusage syscall due to the following > > kernel source: > >asmlinkage long sys_getrusage(int who, struct rusage __user *ru) > >{ > > if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) > > return -EINVAL; > > return getrusage(current, who, ru); > >} > > > >So i want to remove RUSAGE_BOTH from _rusage_who_str. > >Is there anyone knows why RUSAGE_BOTH is necessary, please tell me. > >and if no objection, I will commit it. > > > >Signed-off-by: "Zhaolei" zhaolei@cn.fujitsu.com > > > >--- aux_syscalls.stp.old 2007-09-06 11:51:14.000000000 +0900 > >+++ aux_syscalls.stp 2007-09-06 11:52:22.000000000 +0900 > >@@ -1661,7 +1661,6 @@ function _rlimit_resource_str(r) { > > function _rusage_who_str(w) { > > if(w==0) return "RUSAGE_SELF" > > if(w==-1) return "RUSAGE_CHILDREN" > >- if(w==-2) return "RUSAGE_BOTH" > > return sprintf("UNKNOWN VALUE: %d", w) > > } > > > >Regards > >Zhaolei > > > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] For getrusage's argstr in tapset 2007-09-10 9:28 ` Zhaolei @ 2007-09-10 14:01 ` Wenji Huang 2007-09-13 16:06 ` Zhaolei 0 siblings, 1 reply; 8+ messages in thread From: Wenji Huang @ 2007-09-10 14:01 UTC (permalink / raw) To: Zhaolei; +Cc: systemtap Hi zhaolei, In normal case, thing may be like that. But in abnormal case, maybe wrong arguments are passed to this function. So new entry will give a clearer description, instead of general UNKNOWN. Thanks, Wenji Zhaolei wrote: >Hello, Wenji. > >Thank you for your suggestion. > >I investigated RUSAGE_BOTH and got the following result: >1: _rusage_who_str is only used for sys_getrusage(and compat_sys_getrusage) > in current tapset, and there is no other syscall which needs to call > _rusage_who_str. > >2: RUSAGE_BOTH is only used in kernel's internal function getrusage, but > there is no syscall using this function with RUSAGE_BOTH. > >So I think RUSAGE_BOTH is not necessary in tracing syscall, but it may be > needed when tracing kernel function as getrusage. > >It is good idea disable RUSAGE_BOTH only in sys_getrusage >(and compat_sys_getrusage). > >Regards >Zhaolei >----- Original Message ----- >From: "Wenji Huang" <wenji.huang@oracle.com> >To: "Zhaolei" <zhaolei@cn.fujitsu.com> >Cc: <systemtap@sourceware.org> >Sent: Monday, September 10, 2007 11:44 AM >Subject: Re: [patch] For getrusage's argstr in tapset > > > > >>Hi Zhaolei, >> >> probe getrusage set probe point at the entry of the function. So the >>arguments may be any value. >> >> So I think to keep the entry in _rusage_who_str, just give another >>mapping in case of some certain value. >> >> Of course, if you can ensure that "who" only be >>RUSAGE_SELF/RUSAGE_CHILDREN when the syscall called. Just remove. >> >>Thanks, >>Wenji >> >> >>Zhaolei wrote: >> >> >> >>>Hi, everyone >>> >>>In [tapset->probe getrusage], argstr's "who" argument is printed as >>>"RUSAGE_BOTH" when its value is -2. >>>But "RUSAGE_BOTH" is not used in getrusage syscall due to the following >>>kernel source: >>>asmlinkage long sys_getrusage(int who, struct rusage __user *ru) >>>{ >>> if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) >>> return -EINVAL; >>> return getrusage(current, who, ru); >>>} >>> >>>So i want to remove RUSAGE_BOTH from _rusage_who_str. >>>Is there anyone knows why RUSAGE_BOTH is necessary, please tell me. >>>and if no objection, I will commit it. >>> >>>Signed-off-by: "Zhaolei" zhaolei@cn.fujitsu.com >>> >>>--- aux_syscalls.stp.old 2007-09-06 11:51:14.000000000 +0900 >>>+++ aux_syscalls.stp 2007-09-06 11:52:22.000000000 +0900 >>>@@ -1661,7 +1661,6 @@ function _rlimit_resource_str(r) { >>>function _rusage_who_str(w) { >>> if(w==0) return "RUSAGE_SELF" >>> if(w==-1) return "RUSAGE_CHILDREN" >>>- if(w==-2) return "RUSAGE_BOTH" >>> return sprintf("UNKNOWN VALUE: %d", w) >>>} >>> >>>Regards >>>Zhaolei >>> >>> >>> >>> >>> >> >> > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] For getrusage's argstr in tapset 2007-09-10 14:01 ` Wenji Huang @ 2007-09-13 16:06 ` Zhaolei 0 siblings, 0 replies; 8+ messages in thread From: Zhaolei @ 2007-09-13 16:06 UTC (permalink / raw) To: Wenji Huang; +Cc: systemtap Hello, Wenji. Within the syscall getrusage, there are only 2 types of flag. So in my opinion, all the other values are not getrusage's flag and should be printed as UNKNOWN. To output a clearer description, we need also print the value of flag in the case of "UNKNOWN". So I think it will be better if it can be revised in my way. Regards Zhaolei ----- Original Message ----- From: "Wenji Huang" <wenji.huang@oracle.com> To: "Zhaolei" <zhaolei@cn.fujitsu.com> Cc: <systemtap@sourceware.org> Sent: Monday, September 10, 2007 2:18 PM Subject: Re: [patch] For getrusage's argstr in tapset > Hi zhaolei, > > In normal case, thing may be like that. But in abnormal case, maybe > wrong arguments are passed to this function. So new entry will > give a clearer description, instead of general UNKNOWN. > > Thanks, > Wenji > > Zhaolei wrote: > > >Hello, Wenji. > > > >Thank you for your suggestion. > > > >I investigated RUSAGE_BOTH and got the following result: > >1: _rusage_who_str is only used for sys_getrusage(and compat_sys_getrusage) > > in current tapset, and there is no other syscall which needs to call > > _rusage_who_str. > > > >2: RUSAGE_BOTH is only used in kernel's internal function getrusage, but > > there is no syscall using this function with RUSAGE_BOTH. > > > >So I think RUSAGE_BOTH is not necessary in tracing syscall, but it may be > > needed when tracing kernel function as getrusage. > > > >It is good idea disable RUSAGE_BOTH only in sys_getrusage > >(and compat_sys_getrusage). > > > >Regards > >Zhaolei > >----- Original Message ----- > >From: "Wenji Huang" <wenji.huang@oracle.com> > >To: "Zhaolei" <zhaolei@cn.fujitsu.com> > >Cc: <systemtap@sourceware.org> > >Sent: Monday, September 10, 2007 11:44 AM > >Subject: Re: [patch] For getrusage's argstr in tapset > > > > > > > > > >>Hi Zhaolei, > >> > >> probe getrusage set probe point at the entry of the function. So the > >>arguments may be any value. > >> > >> So I think to keep the entry in _rusage_who_str, just give another > >>mapping in case of some certain value. > >> > >> Of course, if you can ensure that "who" only be > >>RUSAGE_SELF/RUSAGE_CHILDREN when the syscall called. Just remove. > >> > >>Thanks, > >>Wenji > >> > >> > >>Zhaolei wrote: > >> > >> > >> > >>>Hi, everyone > >>> > >>>In [tapset->probe getrusage], argstr's "who" argument is printed as > >>>"RUSAGE_BOTH" when its value is -2. > >>>But "RUSAGE_BOTH" is not used in getrusage syscall due to the following > >>>kernel source: > >>>asmlinkage long sys_getrusage(int who, struct rusage __user *ru) > >>>{ > >>> if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN) > >>> return -EINVAL; > >>> return getrusage(current, who, ru); > >>>} > >>> > >>>So i want to remove RUSAGE_BOTH from _rusage_who_str. > >>>Is there anyone knows why RUSAGE_BOTH is necessary, please tell me. > >>>and if no objection, I will commit it. > >>> > >>>Signed-off-by: "Zhaolei" zhaolei@cn.fujitsu.com > >>> > >>>--- aux_syscalls.stp.old 2007-09-06 11:51:14.000000000 +0900 > >>>+++ aux_syscalls.stp 2007-09-06 11:52:22.000000000 +0900 > >>>@@ -1661,7 +1661,6 @@ function _rlimit_resource_str(r) { > >>>function _rusage_who_str(w) { > >>> if(w==0) return "RUSAGE_SELF" > >>> if(w==-1) return "RUSAGE_CHILDREN" > >>>- if(w==-2) return "RUSAGE_BOTH" > >>> return sprintf("UNKNOWN VALUE: %d", w) > >>>} > >>> > >>>Regards > >>>Zhaolei > >>> > >>> > >>> > >>> > >>> > >> > >> > > > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2007-09-13 7:57 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1188780895.5736.ezmlm@sourceware.org> 2007-09-03 12:48 ` [patch] Modify flag's value in tapset's probe delete_module Zhaolei 2007-09-05 14:46 ` [patch] For memory access error when calling _stp_sockaddr_str with addrlen set to 0 Zhaolei 2007-09-05 22:49 ` Frank Ch. Eigler 2007-09-10 5:03 ` [patch] For getrusage's argstr in tapset Zhaolei 2007-09-10 5:21 ` Wenji Huang 2007-09-10 9:28 ` Zhaolei 2007-09-10 14:01 ` Wenji Huang 2007-09-13 16:06 ` Zhaolei
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 read-only IMAP folder(s) and NNTP newsgroup(s).