diff mbox series

trace point to print ip address we are trying to connect to

Message ID CAH2r5mtoz_droUOS-Uats_WL6MwFY5-pkHRVZo0nUwLdRjOvcA@mail.gmail.com
State New
Headers show
Series trace point to print ip address we are trying to connect to | expand

Commit Message

Steve French Nov. 4, 2021, 6:09 a.m. UTC 
It wasn't obvious to me the best way to pass in a pointer to the ipv4
(and ipv6 address) to a dynamic trace point (unless I create a temp
string first in generic_ip_connect and do the conversion (via "%pI4"
and "%pI6" with sprintf) e.g.

        sprintf(ses->ip_addr, "%pI4", &addr->sin_addr);

The approach I tried passing in the pointer to sin_addr (the
ipv4_address) caused an oops on loading it the first time and the
warning:

[14928.818532] event smb3_ipv4_connect has unsafe dereference of argument 3
[14928.818534] print_fmt: "conn_id=0x%llx server=%s addr=%pI4:%d",
REC->conn_id, __get_str(hostname), REC->ipaddr, REC->port


What I tried was the following (also see attached diff) to print the
ipv4 address that we were trying to connect to

DECLARE_EVENT_CLASS(smb3_connect_class,
TP_PROTO(char *hostname,
__u64 conn_id,
__u16 port,
struct in_addr *ipaddr),
TP_ARGS(hostname, conn_id, port, ipaddr),
TP_STRUCT__entry(
__string(hostname, hostname)
__field(__u64, conn_id)
__field(__u16, port)
__field(const void *, ipaddr)
),
TP_fast_assign(
__entry->port = port;
__entry->conn_id = conn_id;
__entry->ipaddr = ipaddr;
__assign_str(hostname, hostname);
),
TP_printk("conn_id=0x%llx server=%s addr=%pI4:%d",
__entry->conn_id,
__get_str(hostname),
__entry->ipaddr,
__entry->port)
)

#define DEFINE_SMB3_CONNECT_EVENT(name)        \
DEFINE_EVENT(smb3_connect_class, smb3_##name,  \
TP_PROTO(char *hostname, \
__u64 conn_id, \
__u16 port, \
struct in_addr *ipaddr), \
TP_ARGS(hostname, conn_id, port, ipaddr))

DEFINE_SMB3_CONNECT_EVENT(ipv4_connect);

Any ideas how to pass in the ipv4 address - or is it better to convert
it to a string before we call the trace point (which seems wasteful to
me but there must be examples of how to pass in structs to printks in
trace in Linux)

Comments

Stefan Metzmacher Nov. 4, 2021, 7:14 a.m. UTC | #1 
Hi Steve,

you should made this generic (not ipv4/ipv6 specific) and use something like this:

TP_PROTO(const char *hostname, __u64 conn_id, const struct sockaddr_storage *dst_addr)

TP_STRUCT__entry(
__string(hostname, hostname)
__field(__u64, conn_id)
__field(struct sockaddr_storage, dst_addr)

TP_fast_assign(
__entry->conn_id = conn_id;
__entry->dst_addr = *dst_addr;
__assign_str(hostname, hostname);
),

With that you should be able to use this:

TP_printk("conn_id=0x%llx server=%s addr=%pISpsfc",
__entry->conn_id,
__get_str(hostname),
&__entry->dst_addr)

I hope that helps.

metze

Am 04.11.21 um 07:09 schrieb Steve French:
> It wasn't obvious to me the best way to pass in a pointer to the ipv4
> (and ipv6 address) to a dynamic trace point (unless I create a temp
> string first in generic_ip_connect and do the conversion (via "%pI4"
> and "%pI6" with sprintf) e.g.
> 
>         sprintf(ses->ip_addr, "%pI4", &addr->sin_addr);
> 
> The approach I tried passing in the pointer to sin_addr (the
> ipv4_address) caused an oops on loading it the first time and the
> warning:
> 
> [14928.818532] event smb3_ipv4_connect has unsafe dereference of argument 3
> [14928.818534] print_fmt: "conn_id=0x%llx server=%s addr=%pI4:%d",
> REC->conn_id, __get_str(hostname), REC->ipaddr, REC->port
> 
> 
> What I tried was the following (also see attached diff) to print the
> ipv4 address that we were trying to connect to
> 
> DECLARE_EVENT_CLASS(smb3_connect_class,
> TP_PROTO(char *hostname,
> __u64 conn_id,
> __u16 port,
> struct in_addr *ipaddr),
> TP_ARGS(hostname, conn_id, port, ipaddr),
> TP_STRUCT__entry(
> __string(hostname, hostname)
> __field(__u64, conn_id)
> __field(__u16, port)
> __field(const void *, ipaddr)
> ),
> TP_fast_assign(
> __entry->port = port;
> __entry->conn_id = conn_id;
> __entry->ipaddr = ipaddr;
> __assign_str(hostname, hostname);
> ),
> TP_printk("conn_id=0x%llx server=%s addr=%pI4:%d",
> __entry->conn_id,
> __get_str(hostname),
> __entry->ipaddr,
> __entry->port)
> )
> 
> #define DEFINE_SMB3_CONNECT_EVENT(name)        \
> DEFINE_EVENT(smb3_connect_class, smb3_##name,  \
> TP_PROTO(char *hostname, \
> __u64 conn_id, \
> __u16 port, \
> struct in_addr *ipaddr), \
> TP_ARGS(hostname, conn_id, port, ipaddr))
> 
> DEFINE_SMB3_CONNECT_EVENT(ipv4_connect);
> 
> Any ideas how to pass in the ipv4 address - or is it better to convert
> it to a string before we call the trace point (which seems wasteful to
> me but there must be examples of how to pass in structs to printks in
> trace in Linux)
>
Steve French Nov. 4, 2021, 3:09 p.m. UTC | #2 
That looked like a good suggestion and will make the code cleaner but
with that change ran into this unexpected build error.   Ideas?

  CC [M]  /home/smfrench/cifs-2.6/fs/cifs/file.o
In file included from ./include/trace/define_trace.h:102,
                 from /home/smfrench/cifs-2.6/fs/cifs/trace.h:977,
                 from /home/smfrench/cifs-2.6/fs/cifs/trace.c:8:
./include/linux/socket.h:42:26: error: conversion to non-scalar type requested
   42 | #define sockaddr_storage __kernel_sockaddr_storage
      |                          ^~~~~~~~~~~~~~~~~~~~~~~~~
./include/trace/trace_events.h:477:9: note: in definition of macro
‘DECLARE_EVENT_CLASS’
  477 |         tstruct
         \
      |         ^~~~~~~
/home/smfrench/cifs-2.6/fs/cifs/./trace.h:864:9: note: in expansion of
macro ‘TP_STRUCT__entry’
  864 |         TP_STRUCT__entry(
      |         ^~~~~~~~~~~~~~~~
./include/trace/trace_events.h:439:22: note: in expansion of macro
‘is_signed_type’
  439 |         .is_signed = is_signed_type(_type), .filter_type =
_filter_type },
      |                      ^~~~~~~~~~~~~~
./include/trace/trace_events.h:448:33: note: in expansion of macro ‘__field_ext’
  448 | #define __field(type, item)     __field_ext(type, item, FILTER_OTHER)
      |                                 ^~~~~~~~~~~
/home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:17: note: in expansion
of macro ‘__field’
  867 |                 __field(struct sockaddr_storage, dst_addr)
      |                 ^~~~~~~
/home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:32: note: in expansion
of macro ‘sockaddr_storage’
  867 |                 __field(struct sockaddr_storage, dst_addr)

On Thu, Nov 4, 2021 at 2:14 AM Stefan Metzmacher <metze@samba.org> wrote:
>
> Hi Steve,
>
> you should made this generic (not ipv4/ipv6 specific) and use something like this:
>
> TP_PROTO(const char *hostname, __u64 conn_id, const struct sockaddr_storage *dst_addr)
>
> TP_STRUCT__entry(
> __string(hostname, hostname)
> __field(__u64, conn_id)
> __field(struct sockaddr_storage, dst_addr)
>
> TP_fast_assign(
> __entry->conn_id = conn_id;
> __entry->dst_addr = *dst_addr;
> __assign_str(hostname, hostname);
> ),
>
> With that you should be able to use this:
>
> TP_printk("conn_id=0x%llx server=%s addr=%pISpsfc",
> __entry->conn_id,
> __get_str(hostname),
> &__entry->dst_addr)
>
> I hope that helps.
>
> metze
>
> Am 04.11.21 um 07:09 schrieb Steve French:
> > It wasn't obvious to me the best way to pass in a pointer to the ipv4
> > (and ipv6 address) to a dynamic trace point (unless I create a temp
> > string first in generic_ip_connect and do the conversion (via "%pI4"
> > and "%pI6" with sprintf) e.g.
> >
> >         sprintf(ses->ip_addr, "%pI4", &addr->sin_addr);
> >
> > The approach I tried passing in the pointer to sin_addr (the
> > ipv4_address) caused an oops on loading it the first time and the
> > warning:
> >
> > [14928.818532] event smb3_ipv4_connect has unsafe dereference of argument 3
> > [14928.818534] print_fmt: "conn_id=0x%llx server=%s addr=%pI4:%d",
> > REC->conn_id, __get_str(hostname), REC->ipaddr, REC->port
> >
> >
> > What I tried was the following (also see attached diff) to print the
> > ipv4 address that we were trying to connect to
> >
> > DECLARE_EVENT_CLASS(smb3_connect_class,
> > TP_PROTO(char *hostname,
> > __u64 conn_id,
> > __u16 port,
> > struct in_addr *ipaddr),
> > TP_ARGS(hostname, conn_id, port, ipaddr),
> > TP_STRUCT__entry(
> > __string(hostname, hostname)
> > __field(__u64, conn_id)
> > __field(__u16, port)
> > __field(const void *, ipaddr)
> > ),
> > TP_fast_assign(
> > __entry->port = port;
> > __entry->conn_id = conn_id;
> > __entry->ipaddr = ipaddr;
> > __assign_str(hostname, hostname);
> > ),
> > TP_printk("conn_id=0x%llx server=%s addr=%pI4:%d",
> > __entry->conn_id,
> > __get_str(hostname),
> > __entry->ipaddr,
> > __entry->port)
> > )
> >
> > #define DEFINE_SMB3_CONNECT_EVENT(name)        \
> > DEFINE_EVENT(smb3_connect_class, smb3_##name,  \
> > TP_PROTO(char *hostname, \
> > __u64 conn_id, \
> > __u16 port, \
> > struct in_addr *ipaddr), \
> > TP_ARGS(hostname, conn_id, port, ipaddr))
> >
> > DEFINE_SMB3_CONNECT_EVENT(ipv4_connect);
> >
> > Any ideas how to pass in the ipv4 address - or is it better to convert
> > it to a string before we call the trace point (which seems wasteful to
> > me but there must be examples of how to pass in structs to printks in
> > trace in Linux)
> >
>
Steve French Nov. 4, 2021, 3:26 p.m. UTC | #3 
changing it to __kernel_sockaddr_storage the build error is:

                 from /home/smfrench/cifs-2.6/fs/cifs/trace.c:8:
/home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:32: error: conversion to
non-scalar type requested
  867 |                 __field(struct __kernel_sockaddr_storage, dst_addr)
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~
./include/trace/trace_events.h:477:9: note: in definition of macro
‘DECLARE_EVENT_CLASS’
  477 |         tstruct
         \
      |         ^~~~~~~
/home/smfrench/cifs-2.6/fs/cifs/./trace.h:864:9: note: in expansion of
macro ‘TP_STRUCT__entry’
  864 |         TP_STRUCT__entry(

On Thu, Nov 4, 2021 at 10:09 AM Steve French <smfrench@gmail.com> wrote:
>
> That looked like a good suggestion and will make the code cleaner but
> with that change ran into this unexpected build error.   Ideas?
>
>   CC [M]  /home/smfrench/cifs-2.6/fs/cifs/file.o
> In file included from ./include/trace/define_trace.h:102,
>                  from /home/smfrench/cifs-2.6/fs/cifs/trace.h:977,
>                  from /home/smfrench/cifs-2.6/fs/cifs/trace.c:8:
> ./include/linux/socket.h:42:26: error: conversion to non-scalar type requested
>    42 | #define sockaddr_storage __kernel_sockaddr_storage
>       |                          ^~~~~~~~~~~~~~~~~~~~~~~~~
> ./include/trace/trace_events.h:477:9: note: in definition of macro
> ‘DECLARE_EVENT_CLASS’
>   477 |         tstruct
>          \
>       |         ^~~~~~~
> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:864:9: note: in expansion of
> macro ‘TP_STRUCT__entry’
>   864 |         TP_STRUCT__entry(
>       |         ^~~~~~~~~~~~~~~~
> ./include/trace/trace_events.h:439:22: note: in expansion of macro
> ‘is_signed_type’
>   439 |         .is_signed = is_signed_type(_type), .filter_type =
> _filter_type },
>       |                      ^~~~~~~~~~~~~~
> ./include/trace/trace_events.h:448:33: note: in expansion of macro ‘__field_ext’
>   448 | #define __field(type, item)     __field_ext(type, item, FILTER_OTHER)
>       |                                 ^~~~~~~~~~~
> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:17: note: in expansion
> of macro ‘__field’
>   867 |                 __field(struct sockaddr_storage, dst_addr)
>       |                 ^~~~~~~
> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:32: note: in expansion
> of macro ‘sockaddr_storage’
>   867 |                 __field(struct sockaddr_storage, dst_addr)
>
> On Thu, Nov 4, 2021 at 2:14 AM Stefan Metzmacher <metze@samba.org> wrote:
> >
> > Hi Steve,
> >
> > you should made this generic (not ipv4/ipv6 specific) and use something like this:
> >
> > TP_PROTO(const char *hostname, __u64 conn_id, const struct sockaddr_storage *dst_addr)
> >
> > TP_STRUCT__entry(
> > __string(hostname, hostname)
> > __field(__u64, conn_id)
> > __field(struct sockaddr_storage, dst_addr)
> >
> > TP_fast_assign(
> > __entry->conn_id = conn_id;
> > __entry->dst_addr = *dst_addr;
> > __assign_str(hostname, hostname);
> > ),
> >
> > With that you should be able to use this:
> >
> > TP_printk("conn_id=0x%llx server=%s addr=%pISpsfc",
> > __entry->conn_id,
> > __get_str(hostname),
> > &__entry->dst_addr)
> >
> > I hope that helps.
> >
> > metze
> >
> > Am 04.11.21 um 07:09 schrieb Steve French:
> > > It wasn't obvious to me the best way to pass in a pointer to the ipv4
> > > (and ipv6 address) to a dynamic trace point (unless I create a temp
> > > string first in generic_ip_connect and do the conversion (via "%pI4"
> > > and "%pI6" with sprintf) e.g.
> > >
> > >         sprintf(ses->ip_addr, "%pI4", &addr->sin_addr);
> > >
> > > The approach I tried passing in the pointer to sin_addr (the
> > > ipv4_address) caused an oops on loading it the first time and the
> > > warning:
> > >
> > > [14928.818532] event smb3_ipv4_connect has unsafe dereference of argument 3
> > > [14928.818534] print_fmt: "conn_id=0x%llx server=%s addr=%pI4:%d",
> > > REC->conn_id, __get_str(hostname), REC->ipaddr, REC->port
> > >
> > >
> > > What I tried was the following (also see attached diff) to print the
> > > ipv4 address that we were trying to connect to
> > >
> > > DECLARE_EVENT_CLASS(smb3_connect_class,
> > > TP_PROTO(char *hostname,
> > > __u64 conn_id,
> > > __u16 port,
> > > struct in_addr *ipaddr),
> > > TP_ARGS(hostname, conn_id, port, ipaddr),
> > > TP_STRUCT__entry(
> > > __string(hostname, hostname)
> > > __field(__u64, conn_id)
> > > __field(__u16, port)
> > > __field(const void *, ipaddr)
> > > ),
> > > TP_fast_assign(
> > > __entry->port = port;
> > > __entry->conn_id = conn_id;
> > > __entry->ipaddr = ipaddr;
> > > __assign_str(hostname, hostname);
> > > ),
> > > TP_printk("conn_id=0x%llx server=%s addr=%pI4:%d",
> > > __entry->conn_id,
> > > __get_str(hostname),
> > > __entry->ipaddr,
> > > __entry->port)
> > > )
> > >
> > > #define DEFINE_SMB3_CONNECT_EVENT(name)        \
> > > DEFINE_EVENT(smb3_connect_class, smb3_##name,  \
> > > TP_PROTO(char *hostname, \
> > > __u64 conn_id, \
> > > __u16 port, \
> > > struct in_addr *ipaddr), \
> > > TP_ARGS(hostname, conn_id, port, ipaddr))
> > >
> > > DEFINE_SMB3_CONNECT_EVENT(ipv4_connect);
> > >
> > > Any ideas how to pass in the ipv4 address - or is it better to convert
> > > it to a string before we call the trace point (which seems wasteful to
> > > me but there must be examples of how to pass in structs to printks in
> > > trace in Linux)
> > >
> >
>
>
> --
> Thanks,
>
> Steve
Stefan Metzmacher Nov. 4, 2021, 5:02 p.m. UTC | #4 
I looked at my smbdirect dirver code and there I use:

        TP_STRUCT__entry(
                ...
                __array(__u8, addr, sizeof(struct sockaddr_storage))
        ),
        TP_fast_assign(
                pss = (struct sockaddr_storage *)__entry->addr;
                *pss = *addr;
        ),
        TP_printk("... %pISp",
                  __entry->addr)

metze

Am 04.11.21 um 16:26 schrieb Steve French:
> changing it to __kernel_sockaddr_storage the build error is:
> 
>                  from /home/smfrench/cifs-2.6/fs/cifs/trace.c:8:
> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:32: error: conversion to
> non-scalar type requested
>   867 |                 __field(struct __kernel_sockaddr_storage, dst_addr)
>       |                                ^~~~~~~~~~~~~~~~~~~~~~~~~
> ./include/trace/trace_events.h:477:9: note: in definition of macro
> ‘DECLARE_EVENT_CLASS’
>   477 |         tstruct
>          \
>       |         ^~~~~~~
> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:864:9: note: in expansion of
> macro ‘TP_STRUCT__entry’
>   864 |         TP_STRUCT__entry(
> 
> On Thu, Nov 4, 2021 at 10:09 AM Steve French <smfrench@gmail.com> wrote:
>>
>> That looked like a good suggestion and will make the code cleaner but
>> with that change ran into this unexpected build error.   Ideas?
>>
>>   CC [M]  /home/smfrench/cifs-2.6/fs/cifs/file.o
>> In file included from ./include/trace/define_trace.h:102,
>>                  from /home/smfrench/cifs-2.6/fs/cifs/trace.h:977,
>>                  from /home/smfrench/cifs-2.6/fs/cifs/trace.c:8:
>> ./include/linux/socket.h:42:26: error: conversion to non-scalar type requested
>>    42 | #define sockaddr_storage __kernel_sockaddr_storage
>>       |                          ^~~~~~~~~~~~~~~~~~~~~~~~~
>> ./include/trace/trace_events.h:477:9: note: in definition of macro
>> ‘DECLARE_EVENT_CLASS’
>>   477 |         tstruct
>>          \
>>       |         ^~~~~~~
>> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:864:9: note: in expansion of
>> macro ‘TP_STRUCT__entry’
>>   864 |         TP_STRUCT__entry(
>>       |         ^~~~~~~~~~~~~~~~
>> ./include/trace/trace_events.h:439:22: note: in expansion of macro
>> ‘is_signed_type’
>>   439 |         .is_signed = is_signed_type(_type), .filter_type =
>> _filter_type },
>>       |                      ^~~~~~~~~~~~~~
>> ./include/trace/trace_events.h:448:33: note: in expansion of macro ‘__field_ext’
>>   448 | #define __field(type, item)     __field_ext(type, item, FILTER_OTHER)
>>       |                                 ^~~~~~~~~~~
>> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:17: note: in expansion
>> of macro ‘__field’
>>   867 |                 __field(struct sockaddr_storage, dst_addr)
>>       |                 ^~~~~~~
>> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:32: note: in expansion
>> of macro ‘sockaddr_storage’
>>   867 |                 __field(struct sockaddr_storage, dst_addr)
>>
>> On Thu, Nov 4, 2021 at 2:14 AM Stefan Metzmacher <metze@samba.org> wrote:
>>>
>>> Hi Steve,
>>>
>>> you should made this generic (not ipv4/ipv6 specific) and use something like this:
>>>
>>> TP_PROTO(const char *hostname, __u64 conn_id, const struct sockaddr_storage *dst_addr)
>>>
>>> TP_STRUCT__entry(
>>> __string(hostname, hostname)
>>> __field(__u64, conn_id)
>>> __field(struct sockaddr_storage, dst_addr)
>>>
>>> TP_fast_assign(
>>> __entry->conn_id = conn_id;
>>> __entry->dst_addr = *dst_addr;
>>> __assign_str(hostname, hostname);
>>> ),
>>>
>>> With that you should be able to use this:
>>>
>>> TP_printk("conn_id=0x%llx server=%s addr=%pISpsfc",
>>> __entry->conn_id,
>>> __get_str(hostname),
>>> &__entry->dst_addr)
>>>
>>> I hope that helps.
>>>
>>> metze
>>>
>>> Am 04.11.21 um 07:09 schrieb Steve French:
>>>> It wasn't obvious to me the best way to pass in a pointer to the ipv4
>>>> (and ipv6 address) to a dynamic trace point (unless I create a temp
>>>> string first in generic_ip_connect and do the conversion (via "%pI4"
>>>> and "%pI6" with sprintf) e.g.
>>>>
>>>>         sprintf(ses->ip_addr, "%pI4", &addr->sin_addr);
>>>>
>>>> The approach I tried passing in the pointer to sin_addr (the
>>>> ipv4_address) caused an oops on loading it the first time and the
>>>> warning:
>>>>
>>>> [14928.818532] event smb3_ipv4_connect has unsafe dereference of argument 3
>>>> [14928.818534] print_fmt: "conn_id=0x%llx server=%s addr=%pI4:%d",
>>>> REC->conn_id, __get_str(hostname), REC->ipaddr, REC->port
>>>>
>>>>
>>>> What I tried was the following (also see attached diff) to print the
>>>> ipv4 address that we were trying to connect to
>>>>
>>>> DECLARE_EVENT_CLASS(smb3_connect_class,
>>>> TP_PROTO(char *hostname,
>>>> __u64 conn_id,
>>>> __u16 port,
>>>> struct in_addr *ipaddr),
>>>> TP_ARGS(hostname, conn_id, port, ipaddr),
>>>> TP_STRUCT__entry(
>>>> __string(hostname, hostname)
>>>> __field(__u64, conn_id)
>>>> __field(__u16, port)
>>>> __field(const void *, ipaddr)
>>>> ),
>>>> TP_fast_assign(
>>>> __entry->port = port;
>>>> __entry->conn_id = conn_id;
>>>> __entry->ipaddr = ipaddr;
>>>> __assign_str(hostname, hostname);
>>>> ),
>>>> TP_printk("conn_id=0x%llx server=%s addr=%pI4:%d",
>>>> __entry->conn_id,
>>>> __get_str(hostname),
>>>> __entry->ipaddr,
>>>> __entry->port)
>>>> )
>>>>
>>>> #define DEFINE_SMB3_CONNECT_EVENT(name)        \
>>>> DEFINE_EVENT(smb3_connect_class, smb3_##name,  \
>>>> TP_PROTO(char *hostname, \
>>>> __u64 conn_id, \
>>>> __u16 port, \
>>>> struct in_addr *ipaddr), \
>>>> TP_ARGS(hostname, conn_id, port, ipaddr))
>>>>
>>>> DEFINE_SMB3_CONNECT_EVENT(ipv4_connect);
>>>>
>>>> Any ideas how to pass in the ipv4 address - or is it better to convert
>>>> it to a string before we call the trace point (which seems wasteful to
>>>> me but there must be examples of how to pass in structs to printks in
>>>> trace in Linux)
>>>>
>>>
>>
>>
>> --
>> Thanks,
>>
>> Steve
> 
> 
>
Stefan Metzmacher Nov. 4, 2021, 5:07 p.m. UTC | #5 
Am 04.11.21 um 18:02 schrieb Stefan Metzmacher:
> I looked at my smbdirect dirver code and there I use:
> 
>         TP_STRUCT__entry(
>                 ...
>                 __array(__u8, addr, sizeof(struct sockaddr_storage))
>         ),
>         TP_fast_assign(

Here I missed the 'struct sockaddr_storage *pss = NULL;' helper variable...
>                 pss = (struct sockaddr_storage *)__entry->addr;
>                 *pss = *addr;
>         ),
>         TP_printk("... %pISp",
>                   __entry->addr)

My full example is here:

https://git.samba.org/?p=metze/linux/smbdirect.git;a=blob;f=smbdirect_trace.h;hb=refs/heads/smbdirect-work-in-progress#l100

metze

> Am 04.11.21 um 16:26 schrieb Steve French:
>> changing it to __kernel_sockaddr_storage the build error is:
>>
>>                  from /home/smfrench/cifs-2.6/fs/cifs/trace.c:8:
>> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:32: error: conversion to
>> non-scalar type requested
>>   867 |                 __field(struct __kernel_sockaddr_storage, dst_addr)
>>       |                                ^~~~~~~~~~~~~~~~~~~~~~~~~
>> ./include/trace/trace_events.h:477:9: note: in definition of macro
>> ‘DECLARE_EVENT_CLASS’
>>   477 |         tstruct
>>          \
>>       |         ^~~~~~~
>> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:864:9: note: in expansion of
>> macro ‘TP_STRUCT__entry’
>>   864 |         TP_STRUCT__entry(
>>
>> On Thu, Nov 4, 2021 at 10:09 AM Steve French <smfrench@gmail.com> wrote:
>>>
>>> That looked like a good suggestion and will make the code cleaner but
>>> with that change ran into this unexpected build error.   Ideas?
>>>
>>>   CC [M]  /home/smfrench/cifs-2.6/fs/cifs/file.o
>>> In file included from ./include/trace/define_trace.h:102,
>>>                  from /home/smfrench/cifs-2.6/fs/cifs/trace.h:977,
>>>                  from /home/smfrench/cifs-2.6/fs/cifs/trace.c:8:
>>> ./include/linux/socket.h:42:26: error: conversion to non-scalar type requested
>>>    42 | #define sockaddr_storage __kernel_sockaddr_storage
>>>       |                          ^~~~~~~~~~~~~~~~~~~~~~~~~
>>> ./include/trace/trace_events.h:477:9: note: in definition of macro
>>> ‘DECLARE_EVENT_CLASS’
>>>   477 |         tstruct
>>>          \
>>>       |         ^~~~~~~
>>> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:864:9: note: in expansion of
>>> macro ‘TP_STRUCT__entry’
>>>   864 |         TP_STRUCT__entry(
>>>       |         ^~~~~~~~~~~~~~~~
>>> ./include/trace/trace_events.h:439:22: note: in expansion of macro
>>> ‘is_signed_type’
>>>   439 |         .is_signed = is_signed_type(_type), .filter_type =
>>> _filter_type },
>>>       |                      ^~~~~~~~~~~~~~
>>> ./include/trace/trace_events.h:448:33: note: in expansion of macro ‘__field_ext’
>>>   448 | #define __field(type, item)     __field_ext(type, item, FILTER_OTHER)
>>>       |                                 ^~~~~~~~~~~
>>> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:17: note: in expansion
>>> of macro ‘__field’
>>>   867 |                 __field(struct sockaddr_storage, dst_addr)
>>>       |                 ^~~~~~~
>>> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:32: note: in expansion
>>> of macro ‘sockaddr_storage’
>>>   867 |                 __field(struct sockaddr_storage, dst_addr)
>>>
>>> On Thu, Nov 4, 2021 at 2:14 AM Stefan Metzmacher <metze@samba.org> wrote:
>>>>
>>>> Hi Steve,
>>>>
>>>> you should made this generic (not ipv4/ipv6 specific) and use something like this:
>>>>
>>>> TP_PROTO(const char *hostname, __u64 conn_id, const struct sockaddr_storage *dst_addr)
>>>>
>>>> TP_STRUCT__entry(
>>>> __string(hostname, hostname)
>>>> __field(__u64, conn_id)
>>>> __field(struct sockaddr_storage, dst_addr)
>>>>
>>>> TP_fast_assign(
>>>> __entry->conn_id = conn_id;
>>>> __entry->dst_addr = *dst_addr;
>>>> __assign_str(hostname, hostname);
>>>> ),
>>>>
>>>> With that you should be able to use this:
>>>>
>>>> TP_printk("conn_id=0x%llx server=%s addr=%pISpsfc",
>>>> __entry->conn_id,
>>>> __get_str(hostname),
>>>> &__entry->dst_addr)
>>>>
>>>> I hope that helps.
>>>>
>>>> metze
>>>>
>>>> Am 04.11.21 um 07:09 schrieb Steve French:
>>>>> It wasn't obvious to me the best way to pass in a pointer to the ipv4
>>>>> (and ipv6 address) to a dynamic trace point (unless I create a temp
>>>>> string first in generic_ip_connect and do the conversion (via "%pI4"
>>>>> and "%pI6" with sprintf) e.g.
>>>>>
>>>>>         sprintf(ses->ip_addr, "%pI4", &addr->sin_addr);
>>>>>
>>>>> The approach I tried passing in the pointer to sin_addr (the
>>>>> ipv4_address) caused an oops on loading it the first time and the
>>>>> warning:
>>>>>
>>>>> [14928.818532] event smb3_ipv4_connect has unsafe dereference of argument 3
>>>>> [14928.818534] print_fmt: "conn_id=0x%llx server=%s addr=%pI4:%d",
>>>>> REC->conn_id, __get_str(hostname), REC->ipaddr, REC->port
>>>>>
>>>>>
>>>>> What I tried was the following (also see attached diff) to print the
>>>>> ipv4 address that we were trying to connect to
>>>>>
>>>>> DECLARE_EVENT_CLASS(smb3_connect_class,
>>>>> TP_PROTO(char *hostname,
>>>>> __u64 conn_id,
>>>>> __u16 port,
>>>>> struct in_addr *ipaddr),
>>>>> TP_ARGS(hostname, conn_id, port, ipaddr),
>>>>> TP_STRUCT__entry(
>>>>> __string(hostname, hostname)
>>>>> __field(__u64, conn_id)
>>>>> __field(__u16, port)
>>>>> __field(const void *, ipaddr)
>>>>> ),
>>>>> TP_fast_assign(
>>>>> __entry->port = port;
>>>>> __entry->conn_id = conn_id;
>>>>> __entry->ipaddr = ipaddr;
>>>>> __assign_str(hostname, hostname);
>>>>> ),
>>>>> TP_printk("conn_id=0x%llx server=%s addr=%pI4:%d",
>>>>> __entry->conn_id,
>>>>> __get_str(hostname),
>>>>> __entry->ipaddr,
>>>>> __entry->port)
>>>>> )
>>>>>
>>>>> #define DEFINE_SMB3_CONNECT_EVENT(name)        \
>>>>> DEFINE_EVENT(smb3_connect_class, smb3_##name,  \
>>>>> TP_PROTO(char *hostname, \
>>>>> __u64 conn_id, \
>>>>> __u16 port, \
>>>>> struct in_addr *ipaddr), \
>>>>> TP_ARGS(hostname, conn_id, port, ipaddr))
>>>>>
>>>>> DEFINE_SMB3_CONNECT_EVENT(ipv4_connect);
>>>>>
>>>>> Any ideas how to pass in the ipv4 address - or is it better to convert
>>>>> it to a string before we call the trace point (which seems wasteful to
>>>>> me but there must be examples of how to pass in structs to printks in
>>>>> trace in Linux)
>>>>>
>>>>
>>>
>>>
>>> --
>>> Thanks,
>>>
>>> Steve
>>
>>
>>
> 
>
Steve French Nov. 4, 2021, 9:05 p.m. UTC | #6 
Here is the updated patch - seems to work. See attached.

#                              |||| /     delay
#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
      mount.cifs-14551   [005] .....  7636.547906: smb3_connect_done:
conn_id=0x1 server=localhost addr=127.0.0.1:445
      mount.cifs-14558   [004] .....  7642.405413: smb3_connect_done:
conn_id=0x2 server=smfrench.file.core.windows.net
addr=52.239.158.232:445
      mount.cifs-14741   [005] .....  7818.490716: smb3_connect_done:
conn_id=0x3 server=::1 addr=[::1]:445/0%0
      mount.cifs-14810   [000] .....  7966.380337: smb3_connect_err:
rc=-101 conn_id=0x4 server=::2 addr=[::2]:445/0%0
      mount.cifs-14810   [000] .....  7966.380356: smb3_connect_err:
rc=-101 conn_id=0x4 server=::2 addr=[::2]:139/0%0
      mount.cifs-14818   [003] .....  7986.771992: smb3_connect_done:
conn_id=0x5 server=127.0.0.9 addr=127.0.0.9:445
      mount.cifs-14825   [008] .....  8008.178109: smb3_connect_err:
rc=-115 conn_id=0x6 server=124.23.0.9 addr=124.23.0.9:445
      mount.cifs-14825   [008] .....  8013.298085: smb3_connect_err:
rc=-115 conn_id=0x6 server=124.23.0.9 addr=124.23.0.9:139
           cifsd-14553   [006] .....  8036.735615: smb3_reconnect:
conn_id=0x1 server=localhost current_mid=32
           cifsd-14743   [010] .....  8036.735644: smb3_reconnect:
conn_id=0x3 server=::1 current_mid=29
           cifsd-14743   [010] .....  8036.735686: smb3_connect_err:
rc=-111 conn_id=0x3 server=::1 addr=[::1]:445/0%0
           cifsd-14553   [008] .....  8036.737867: smb3_connect_err:
rc=-111 conn_id=0x1 server=localhost addr=127.0.0.1:445
           cifsd-14743   [010] .....  8039.921740: smb3_connect_err:
rc=-111 conn_id=0x3 server=::1 addr=[::1]:445/0%0
           cifsd-14553   [008] .....  8039.921743: smb3_connect_err:
rc=-111 conn_id=0x1 server=localhost addr=127.0.0.1:445
           cifsd-14553   [008] .....  8042.993894: smb3_connect_err:
rc=-111 conn_id=0x1 server=localhost addr=127.0.0.1:445
           cifsd-14743   [010] .....  8042.993894: smb3_connect_err:
rc=-111 conn_id=0x3 server=::1 addr=[::1]:445/0%0
           cifsd-14553   [008] .....  8046.065824: smb3_connect_err:
rc=-111 conn_id=0x1 server=localhost addr=127.0.0.1:445
           cifsd-14743   [010] .....  8046.065824: smb3_connect_err:
rc=-111 conn_id=0x3 server=::1 addr=[::1]:445/0%0
           cifsd-14553   [008] .....  8049.137796: smb3_connect_done:
conn_id=0x1 server=localhost addr=127.0.0.1:445
           cifsd-14743   [010] .....  8049.137796: smb3_connect_done:
conn_id=0x3 server=::1 addr=[::1]:445/0%0

On Thu, Nov 4, 2021 at 12:07 PM Stefan Metzmacher <metze@samba.org> wrote:
>
> Am 04.11.21 um 18:02 schrieb Stefan Metzmacher:
> > I looked at my smbdirect dirver code and there I use:
> >
> >         TP_STRUCT__entry(
> >                 ...
> >                 __array(__u8, addr, sizeof(struct sockaddr_storage))
> >         ),
> >         TP_fast_assign(
>
> Here I missed the 'struct sockaddr_storage *pss = NULL;' helper variable...
> >                 pss = (struct sockaddr_storage *)__entry->addr;
> >                 *pss = *addr;
> >         ),
> >         TP_printk("... %pISp",
> >                   __entry->addr)
>
> My full example is here:
>
> https://git.samba.org/?p=metze/linux/smbdirect.git;a=blob;f=smbdirect_trace.h;hb=refs/heads/smbdirect-work-in-progress#l100
>
> metze
>
> > Am 04.11.21 um 16:26 schrieb Steve French:
> >> changing it to __kernel_sockaddr_storage the build error is:
> >>
> >>                  from /home/smfrench/cifs-2.6/fs/cifs/trace.c:8:
> >> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:32: error: conversion to
> >> non-scalar type requested
> >>   867 |                 __field(struct __kernel_sockaddr_storage, dst_addr)
> >>       |                                ^~~~~~~~~~~~~~~~~~~~~~~~~
> >> ./include/trace/trace_events.h:477:9: note: in definition of macro
> >> ‘DECLARE_EVENT_CLASS’
> >>   477 |         tstruct
> >>          \
> >>       |         ^~~~~~~
> >> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:864:9: note: in expansion of
> >> macro ‘TP_STRUCT__entry’
> >>   864 |         TP_STRUCT__entry(
> >>
> >> On Thu, Nov 4, 2021 at 10:09 AM Steve French <smfrench@gmail.com> wrote:
> >>>
> >>> That looked like a good suggestion and will make the code cleaner but
> >>> with that change ran into this unexpected build error.   Ideas?
> >>>
> >>>   CC [M]  /home/smfrench/cifs-2.6/fs/cifs/file.o
> >>> In file included from ./include/trace/define_trace.h:102,
> >>>                  from /home/smfrench/cifs-2.6/fs/cifs/trace.h:977,
> >>>                  from /home/smfrench/cifs-2.6/fs/cifs/trace.c:8:
> >>> ./include/linux/socket.h:42:26: error: conversion to non-scalar type requested
> >>>    42 | #define sockaddr_storage __kernel_sockaddr_storage
> >>>       |                          ^~~~~~~~~~~~~~~~~~~~~~~~~
> >>> ./include/trace/trace_events.h:477:9: note: in definition of macro
> >>> ‘DECLARE_EVENT_CLASS’
> >>>   477 |         tstruct
> >>>          \
> >>>       |         ^~~~~~~
> >>> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:864:9: note: in expansion of
> >>> macro ‘TP_STRUCT__entry’
> >>>   864 |         TP_STRUCT__entry(
> >>>       |         ^~~~~~~~~~~~~~~~
> >>> ./include/trace/trace_events.h:439:22: note: in expansion of macro
> >>> ‘is_signed_type’
> >>>   439 |         .is_signed = is_signed_type(_type), .filter_type =
> >>> _filter_type },
> >>>       |                      ^~~~~~~~~~~~~~
> >>> ./include/trace/trace_events.h:448:33: note: in expansion of macro ‘__field_ext’
> >>>   448 | #define __field(type, item)     __field_ext(type, item, FILTER_OTHER)
> >>>       |                                 ^~~~~~~~~~~
> >>> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:17: note: in expansion
> >>> of macro ‘__field’
> >>>   867 |                 __field(struct sockaddr_storage, dst_addr)
> >>>       |                 ^~~~~~~
> >>> /home/smfrench/cifs-2.6/fs/cifs/./trace.h:867:32: note: in expansion
> >>> of macro ‘sockaddr_storage’
> >>>   867 |                 __field(struct sockaddr_storage, dst_addr)
> >>>
> >>> On Thu, Nov 4, 2021 at 2:14 AM Stefan Metzmacher <metze@samba.org> wrote:
> >>>>
> >>>> Hi Steve,
> >>>>
> >>>> you should made this generic (not ipv4/ipv6 specific) and use something like this:
> >>>>
> >>>> TP_PROTO(const char *hostname, __u64 conn_id, const struct sockaddr_storage *dst_addr)
> >>>>
> >>>> TP_STRUCT__entry(
> >>>> __string(hostname, hostname)
> >>>> __field(__u64, conn_id)
> >>>> __field(struct sockaddr_storage, dst_addr)
> >>>>
> >>>> TP_fast_assign(
> >>>> __entry->conn_id = conn_id;
> >>>> __entry->dst_addr = *dst_addr;
> >>>> __assign_str(hostname, hostname);
> >>>> ),
> >>>>
> >>>> With that you should be able to use this:
> >>>>
> >>>> TP_printk("conn_id=0x%llx server=%s addr=%pISpsfc",
> >>>> __entry->conn_id,
> >>>> __get_str(hostname),
> >>>> &__entry->dst_addr)
> >>>>
> >>>> I hope that helps.
> >>>>
> >>>> metze
> >>>>
> >>>> Am 04.11.21 um 07:09 schrieb Steve French:
> >>>>> It wasn't obvious to me the best way to pass in a pointer to the ipv4
> >>>>> (and ipv6 address) to a dynamic trace point (unless I create a temp
> >>>>> string first in generic_ip_connect and do the conversion (via "%pI4"
> >>>>> and "%pI6" with sprintf) e.g.
> >>>>>
> >>>>>         sprintf(ses->ip_addr, "%pI4", &addr->sin_addr);
> >>>>>
> >>>>> The approach I tried passing in the pointer to sin_addr (the
> >>>>> ipv4_address) caused an oops on loading it the first time and the
> >>>>> warning:
> >>>>>
> >>>>> [14928.818532] event smb3_ipv4_connect has unsafe dereference of argument 3
> >>>>> [14928.818534] print_fmt: "conn_id=0x%llx server=%s addr=%pI4:%d",
> >>>>> REC->conn_id, __get_str(hostname), REC->ipaddr, REC->port
> >>>>>
> >>>>>
> >>>>> What I tried was the following (also see attached diff) to print the
> >>>>> ipv4 address that we were trying to connect to
> >>>>>
> >>>>> DECLARE_EVENT_CLASS(smb3_connect_class,
> >>>>> TP_PROTO(char *hostname,
> >>>>> __u64 conn_id,
> >>>>> __u16 port,
> >>>>> struct in_addr *ipaddr),
> >>>>> TP_ARGS(hostname, conn_id, port, ipaddr),
> >>>>> TP_STRUCT__entry(
> >>>>> __string(hostname, hostname)
> >>>>> __field(__u64, conn_id)
> >>>>> __field(__u16, port)
> >>>>> __field(const void *, ipaddr)
> >>>>> ),
> >>>>> TP_fast_assign(
> >>>>> __entry->port = port;
> >>>>> __entry->conn_id = conn_id;
> >>>>> __entry->ipaddr = ipaddr;
> >>>>> __assign_str(hostname, hostname);
> >>>>> ),
> >>>>> TP_printk("conn_id=0x%llx server=%s addr=%pI4:%d",
> >>>>> __entry->conn_id,
> >>>>> __get_str(hostname),
> >>>>> __entry->ipaddr,
> >>>>> __entry->port)
> >>>>> )
> >>>>>
> >>>>> #define DEFINE_SMB3_CONNECT_EVENT(name)        \
> >>>>> DEFINE_EVENT(smb3_connect_class, smb3_##name,  \
> >>>>> TP_PROTO(char *hostname, \
> >>>>> __u64 conn_id, \
> >>>>> __u16 port, \
> >>>>> struct in_addr *ipaddr), \
> >>>>> TP_ARGS(hostname, conn_id, port, ipaddr))
> >>>>>
> >>>>> DEFINE_SMB3_CONNECT_EVENT(ipv4_connect);
> >>>>>
> >>>>> Any ideas how to pass in the ipv4 address - or is it better to convert
> >>>>> it to a string before we call the trace point (which seems wasteful to
> >>>>> me but there must be examples of how to pass in structs to printks in
> >>>>> trace in Linux)
> >>>>>
> >>>>
> >>>
> >>>
> >>> --
> >>> Thanks,
> >>>
> >>> Steve
> >>
> >>
> >>
> >
> >
>
>
diff mbox series

Patch

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index e6e261dfd107..4cbfca90a47c 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2590,6 +2590,8 @@  generic_ip_connect(struct TCP_Server_Info *server)
 		sfamily = AF_INET;
 		cifs_dbg(FYI, "%s: connecting to %pI4:%d\n", __func__, &ipv4->sin_addr,
 				ntohs(sport));
+		trace_smb3_ipv4_connect(server->hostname, server->conn_id,
+					ntohs(sport), &ipv4->sin_addr);
 	}
 
 	if (socket == NULL) {
diff --git a/fs/cifs/trace.h b/fs/cifs/trace.h
index dafcb6ab050d..3162f484fb85 100644
--- a/fs/cifs/trace.h
+++ b/fs/cifs/trace.h
@@ -11,7 +11,7 @@ 
 #define _CIFS_TRACE_H
 
 #include <linux/tracepoint.h>
-
+#include <linux/inet.h>
 /*
  * Please use this 3-part article as a reference for writing new tracepoints:
  * https://lwn.net/Articles/379903/
@@ -854,6 +854,41 @@  DEFINE_EVENT(smb3_lease_err_class, smb3_##name,  \
 
 DEFINE_SMB3_LEASE_ERR_EVENT(lease_err);
 
+DECLARE_EVENT_CLASS(smb3_connect_class,
+	TP_PROTO(char *hostname,
+		__u64 conn_id,
+		__u16 port,
+		struct in_addr *ipaddr),
+	TP_ARGS(hostname, conn_id, port, ipaddr),
+	TP_STRUCT__entry(
+		__string(hostname, hostname)
+		__field(__u64, conn_id)
+		__field(__u16, port)
+		__field(const void *, ipaddr)
+	),
+	TP_fast_assign(
+		__entry->port = port;
+		__entry->conn_id = conn_id;
+		__entry->ipaddr = ipaddr;
+		__assign_str(hostname, hostname);
+	),
+	TP_printk("conn_id=0x%llx server=%s addr=%pI4:%d",
+		__entry->conn_id,
+		__get_str(hostname),
+		__entry->ipaddr,
+		__entry->port)
+)
+
+#define DEFINE_SMB3_CONNECT_EVENT(name)        \
+DEFINE_EVENT(smb3_connect_class, smb3_##name,  \
+	TP_PROTO(char *hostname,		\
+		__u64 conn_id,			\
+		__u16 port,			\
+		struct in_addr *ipaddr),	\
+	TP_ARGS(hostname, conn_id, port, ipaddr))
+
+DEFINE_SMB3_CONNECT_EVENT(ipv4_connect);
+
 DECLARE_EVENT_CLASS(smb3_reconnect_class,
 	TP_PROTO(__u64	currmid,
 		__u64 conn_id,