[patch] Modify flag's value in tapset's probe delete_module

[patch] Modify flag's value in tapset's probe delete_module

[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

[patch] For memory access error when calling _stp_sockaddr_str with addrlen set to 0

[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

Re: [patch] For memory access error when calling _stp_sockaddr_str with addrlen set to 0

[Frank Ch. Eigler]

"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

[patch] For getrusage's argstr in tapset

[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

Re: [patch] For getrusage's argstr in tapset

[Wenji Huang]

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
>
>  
>

Re: [patch] For getrusage's argstr in tapset

[Zhaolei]

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
> >
> >  
> >
> 
> 

Re: [patch] For getrusage's argstr in tapset

[Wenji Huang]

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
>>>
>>> 
>>>
>>>      
>>>
>>    
>>
>
>  
>

Re: [patch] For getrusage's argstr in tapset

[Zhaolei]

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
> >>>
> >>> 
> >>>
> >>>      
> >>>
> >>    
> >>
> >
> >  
> >
> 
>