Replace PID with Task Pointer in Rusty

[likewhatevs]

I wasnt't able to reopen #672 because I force pushed (just learned about that limit).

This is a continuation of that PR.

I closed that PR and opened #710 because, while the first PR's approach passed stress tests and seemed to work, it kind of didn't make sense that I could deref the casted pointer to task_struct.

I updated that initial PR to explicltly deref that casted pointer to confirm this works/the verifier allows it and it seems to work so, this works?

see latest comment

[vax-r]

Wouldn't this design throw away the origin safety provided by bpf_task_from_pid() ? It was able to perform reference count on the task_struct .
Using a map to store is awesome but it seems that we do not have mechanism to cleanup the unused <k, v> pair in the map, as the time going, the map size can grow really big, even it's acceptable, we would still like to avoid redundant memory usage , maybe having that implemented will be nice?

[vax-r]

Btw I want to ask what's the actual cause of Failed to lookup task %d ?

[likewhatevs]

Wouldn't this design throw away the origin safety provided by bpf_task_from_pid() ? It was able to perform reference count on the task_struct .

That was not apparent to me but now that you mention it, this sounds like it might be what I've been fighting the verifier over wrt/ all the different attempts to make this work (i.e. for every attempt, once the code makes sense, instruction count skyrockets and the verifier refuses to pass this code).

Using a map to store is awesome but it seems that we do not have mechanism to cleanup the unused <k, v> pair in the map, as the time going, the map size can grow really big, even it's acceptable, we would still like to avoid redundant memory usage , maybe having that implemented will be nice?

I'm going to give that a try using bpf_task_from_pid and see if that can get the verifier to allow this code to run.

Btw I want to ask what's the actual cause of Failed to lookup task %d ?

tl;dr -- pid's can change (but task struct ptrs cannot): #610

[likewhatevs]

I think this is the best we (or well, at least I) can do atm.

So like, I tried a few ways of tracking the association between the initial pid of a task struct and the pointer of the task struct.

My most recent attempts errored at null-checking the task context read using that association:

; if (!taskc) goto free_task; @ main.bpf.c:456
372: (15) if r3 == 0x0 goto pc+27
R3 !read_ok
processed 11065 insns (limit 1000000) max_states_per_insn 8 total_states 740 peak_states 298 mark_read 18
-- END PROG LOAD LOG --

Unless I'm reading something wrong or misunderstand something, I think this is the best we can do ATM (i.e. until it's possible to null check the retrieved variable).

I think this might be alright though because like, tests do pass and logs on those tests do not have stalls (which, I think, is the main reason to be concerned we are tracking everything).

[vax-r]

I think this might be alright though because like, tests do pass and logs on those tests do not have stalls (which, I think, is the main reason to be concerned we are tracking everything).

It seems to me that what the commit does is only refactor the origin code , rather than actually changing any logic or mechanism when dealing with pid and task_struct, how can it pass this time ? It seems confusing to me.

[likewhatevs]

It seems to me that what the commit does is only refactor the origin code , rather than actually changing any logic or mechanism when dealing with pid and task_struct, how can it pass this time ? It seems confusing to me.

This is correct. What I am wondering is if the current behavior (without that exception being thrown) is problematic. I would think I would see a stall if it were if I understand how things work correctly now, but I'm not super certain of that.

[hodgesds]

This is correct. What I am wondering is if the current behavior (without that exception being thrown) is problematic. I would think I would see a stall if it were if I understand how things work correctly now, but I'm not super certain of that.

I think the idea is that if correct scheduling decisions can't be made by the the scheduler it's better to exit (the scheduler) rather than making bad scheduling decisions. There's still some bugs in rusty, so in some ways it maybe is better to fail fast in more places. In this case of looking up the task context I think it's probably ok to bail the scheduler, because if the task context is lost/incorrect then making correct scheduling decisions is probably not handled correctly elsewhere.

[likewhatevs]

I think the idea is that if correct scheduling decisions can't be made by the the scheduler it's better to exit (the scheduler) rather than making bad scheduling decisions.

That makes sense.

In this case of looking up the task context I think it's probably ok to bail the scheduler, because if the task context is lost/incorrect then making correct scheduling decisions is probably not handled correctly elsewhere.

This is the part where like, IDK. So like, if we lost the task, it wouldn't be scheduled, and we would see a stall?

If that is correct, then maybe (like, super maybe, idk), we're getting the task again after the pid change happens, and because we're keying off the PID, we're treating task as an entirely new task (hence no stall)?

[hodgesds]

This is the part where like, IDK. So like, if we lost the task, it wouldn't be scheduled, and we would see a stall?

I think the problem with stress-ng tests is that with the --fork tests the tasks come and go really fast, I'm not sure if there's an easy way to trigger the behavior (maybe compiling the kernel like in #684) and see what happens with this change. Maybe do a kernel build with something crazy like ````make -j $(nproc * 2)```.

[likewhatevs]

I think the problem with stress-ng tests is that with the --fork tests the tasks come and go really fast, I'm not sure if there's an easy way to trigger the behavior (maybe compiling the kernel like in #684) and see what happens with this change. Maybe do a kernel build with something crazy like ````make -j $(nproc * 2)```.

gonna run it on laptop for a day and do the terraria/compile test/see if I can find bad behavior, good idea, thx!

[likewhatevs]

Alright so I think I finally got this (and learned a ton along the way).

This PR now has Rusty use Task pointer casted to u64 to track tasks instead of pid.

A couple things I learned:

In bpf, you can't cast "trusted pointers". I don't fully get it, but t_to_tptr makes the cast work w/ the verifier.

Making functions static makes some problems go away, but can also super quickly drive things over the instruction limit.

More interestingly, I tried only grabbing pid via probe_read (rather than a pointer to task_struct), and to then grab task_struct via bpf_task_from_pid (because maybe the bpf locking could be useful/I was curious).

What I found is that is super racy (i.e. adding a counter and a while loop made stress test last longer while before failing with a missing pid error), so I kind of think we need to not use bpf_task_from_pid anywhere.

[hodgesds]

I think this will work!