[DTrace-devel] [PATCH 3/6] dtprobed: support DOF > 64KiB

[Eugene Loh]

On 5/25/23 16:55, Nick Alcock wrote:

> On 23 May 2023, Eugene Loh via DTrace-devel spake thusly:
>> On 5/22/23 16:20, Nick Alcock via DTrace-devel wrote:
>>> -		if (in_bufsz > sizeof(dof_hdr_t)) {
>>> -			errmsg = "DOF header size incorrect";
>>> -			fuse_log(FUSE_LOG_ERR, "%i: dtprobed: %s: %zi, not %zi\n",
>>> -				 pid, errmsg, in_bufsz, sizeof(dof_hdr_t));
>>> -			goto fuse_err;
>>> +		if (userdata->state == DTP_IOCTL_DOFHDR &&
>> Why introduce the userdata->state==DTP_IOCTL_DOFHDR check when this check was just performed?  Why do it twice?
> Before this patch, we are repeatedly called back by FUSE like so:
>
> call 0: DTP_IOCTL_START: just a pointer, directly from drti.c; ask FUSE
>                           to deref it
> call 1: DTP_IOCTL_HDR: dereffed to get a dof_helper_t; this contains a
>                         pointer to the DOF itself; ask FUSE to deref it;
>                         we cannot yet tell how big it is, but it must be
>                         at least as big as a dof_hdr_t, so get that much
> call 2: DTP_IOCTL_DOFHDR: dereffed to get a dof_hdr_t; now we know how
>                            big the DOF is; ask FUSE to get that much
>                            from the same address
> call 3: DTP_IOCTL_DOF: we have all the DOF! parse it
>
> After this patch, we add a state, and call 2 above becomes:
>
> call 2: DTP_IOCTL_DOFHDR: dereffed to get a dof_hdr_t; now we know how
>                            big the DOF is; ask FUSE to get that much
>                            from the same address; if it's bigger than
>                            64K, ask FUSE to get the first 64K
> call 2+: DTP_IOCTL_DOFCHUNK: called back with 64K more of a >64K DOF;
>                               concatenate it and possibly get the next
>                               64K. Once all are done, go on to
>                               DTP_IOCTL_DOF as above.
>
> I would be entirely happy to add more comments explaining this better.
> Clearly my current ones aren't good enough :(

Just in case we're looking at different things, I'm looking at:

         /*
          * Third call: validate the DOF length and stash it away.
          */
         if (userdata->state == DTP_IOCTL_DOFHDR) {
                 /*
                  * Too much data is as bad as too little.
                  */
                 if (userdata->state == DTP_IOCTL_DOFHDR &&
                     (in_bufsz > sizeof(dof_hdr_t))) {

It looks like we're checking userdata->state==DTP_IOCTL_DOFHDR twice.

>>> +script
>>> +status=$?
>>> +
>>> +# Temporary, until autocleanup is implemented
>>> +echo - > /sys/kernel/tracing/uprobe_events 2>/dev/null
>> Doesn't that remove all uprobes?
> ... possibly breaking other USDT users on the same system as the test?
> Yes, I suppose it does, but I'm not sure how to remove only the huge
> pile of now-redundant uprobes this test leaves behind, and if you don't
> the remaining tests become incredibly slow. (Modern systems, tens of
> billions of clock cycles per second and we can't have as many probes as
> we had bytes of RAM in C64 BASIC without the system hitting a wall.
> Sheesh.)
>
> Got some syntax that might help? (And possibly a way to fix this stupid
> problem for good? dtprobed should be cleaning this crap up somehow, but
> it's tricky because just because the original process is dead doesn't
> mean some of the probes in it, particularly in shared libraries, don't
> survive in accessible mappings in other processes.)

Two things:

1)  It's /sys/kernel/debug/tracing/uprobe_events.  (You're missing 
"debug/".)

2)  The uprobes added by the test *are* recognizable, so you could 
remove them one-by-one if necessary.  E.g., worst case, look at the 
contents of uprobe_events and search on the right pid or something. A 
little messy, but so be it.