On 18/07/2023 14.45, Luben Tuikov wrote:
On 2023-07-17 22:35, Asahi Lina wrote:On 18/07/2023 00.55, Christian König wrote:Am 15.07.23 um 16:14 schrieb alyssa@xxxxxxxxxxxxx:15 July 2023 at 00:03, "Luben Tuikov" <luben.tuikov@xxxxxxx> wrote:On 2023-07-14 05:57, Christian König wrote:Am 14.07.23 um 11:49 schrieb Asahi Lina:On 14/07/2023 17.43, Christian König wrote:Am 14.07.23 um 10:21 schrieb Asahi Lina: A signaled scheduler fence can outlive its scheduler, since fences are independencly reference counted. Therefore, we can't reference the scheduler in the get_timeline_name() implementation. Fixes oopses on `cat /sys/kernel/debug/dma_buf/bufinfo` when shared dma-bufs reference fences from GPU schedulers that no longer exist. Signed-off-by: Asahi Lina <lina@xxxxxxxxxxxxx> --- drivers/gpu/drm/scheduler/sched_entity.c | 7 ++++++- drivers/gpu/drm/scheduler/sched_fence.c | 4 +++- include/drm/gpu_scheduler.h | 5 +++++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index b2bbc8a68b30..17f35b0b005a 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -389,7 +389,12 @@ static bool drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity) /* * Fence is from the same scheduler, only need to wait for - * it to be scheduled + * it to be scheduled. + * + * Note: s_fence->sched could have been freed and reallocated + * as another scheduler. This false positive case is okay, as if + * the old scheduler was freed all of its jobs must have + * signaled their completion fences. This is outright nonsense. As long as an entity for a scheduler exists it is not allowed to free up this scheduler. So this function can't be called like this.Well this is function is not about fences, this function is a callbackAs I already explained, the fences can outlive their scheduler. That means *this* entity certainly exists for *this* scheduler, but the *dependency* fence might have come from a past scheduler that was already destroyed along with all of its entities, and its address reused.for the entity.I'm certainly trying to help here, but you seem to have unrealisticChristian, I'm really getting tired of your tone. I don't appreciate being told my comments are "outright nonsense" when you don't even take the time to understand what the issue is and what I'm trying to do/document. If you aren't interested in working with me, I'm just going to give up on drm_sched, wait until Rust gets workqueue support, and reimplement it in Rust. You can keep your broken fence lifetime semantics and I'll do my own thing.expectations.I perfectly understand what you are trying to do, but you don't seem tounderstand that this functionality here isn't made for your use case.We can adjust the functionality to better match your requirements, butyou can't say it is broken because it doesn't work when you use it not in the way it is intended to be used.I believe "adjusting" functionality to fit some external requirements, may have unintended consequences, requiring yet more and more "adjustments". (Or may allow (new) drivers to do wild things which may lead to wild results. :-) ) We need to be extra careful and wary of this.Either drm/scheduler is common code that we should use for our driver, in which case we need to "adjust" it to fit the requirements of a safe Rust abstraction usable for AGX.Well this is the fundamental disagreement we have. As far as I can see you don't need to adjust anything in the common drm/scheduler code. That code works with quite a bunch of different drivers, including the Intel XE which has similar requirements to your work here. We can talk about gradually improving the common code, but as Luben already wrote as well this needs to be done very carefully.Or, drm/scheduler is not common code intended for drivers with our requirements, and then we need to be able to write our own scheduler. AMD has NAK'd both options, effectively NAK'ing the driver. I will ask a simple yes/no question: Should we use drm/sched?Well, yes.If yes, it will need patches like these,No, you don't. First of all you need to try to adjust your driver to match the requirements of drm/scheduler and *not* the other way around.and AMD needs to be ok with that and stop NAK'ing them on sight becuase they don't match the existing requirements. If no, we will write our own scheduler in Rust, and AMD needs to be ok with that and not NAK it on sight because it's not drm/sched. Which is it? Note if we write a Rust scheduler, drm/sched and amdgpu will be unaffected. If we do that and AMD comes back and NAKs it -- as said in this thread would "probably" happen -- then it is impossible for us to upstream a driver regardless of whether we use drm/sched. Lina has been polite and accommodating while AMD calls her code "outright nonsense" and gets "outright NAK"s, and puts her into an impossible catch-22 where no matter what she does it's NAK'd.Well as far as I can see I'm totally polite as well. Pointing out that approaches doesn't seem to make sense and NAKing patches is a perfectly normal part of the review process. What you need to to is to take a step back and ask yourself why this here is facing so much rejection from our side. I have to admit that I don't seem to be good at explaining that, cause we are obviously talking past each other, but you don't seem to try hard to understand what I'm pointing out either.That's not ok.As far as I can see it is. As maintainer of a commonly used component my first duty is to preserve the status quo and prevent modifications which are not well thought through. And to be honest those changes here strongly looks like Lina is just adjusting the code to match her requirements without looking left and right first. Regards, Christian.I give up. You are ignoring everything we say, and rejecting everything we suggest. We've already explained why drm_sched doesn't work for us. I'm tired of repeating the same explanation over and over again only to be ignored and told I'm wrong. I'll start working on a new, much simpler Rust-native scheduler based on the workqueue Rust abstractions which are in review. ~~ LinaPerhaps it is worth having a reset and just trying to clarify requirements one at a time, even if that involves describing a change on a single line in a single file.
I've already tried to explain the issue. The DRM scheduler design, as it stands today, makes it impractical to write a safe Rust abstraction for it. This is a fact. Christian has repeatedly NAKed my attempts at changing it to make such a safe abstraction possible, and is clearly opposed to the fundamental lifetime requirements change I am trying to make here. Therefore, we are at an impasse.
It's unfortunate, but given this situation, the DRM scheduler will not be available to Rust DRM drivers. I thought the goal of the DRM subsystem common code was to cater to multiple drivers and usage approaches, with an emphasis on doing things "right" and avoiding design issues that are common mistakes in driver design. Clearly, this is not the case for all of DRM, at least not the DRM scheduler.
In software engineering, complex lifetime requirements are an anti-pattern, which is one reason why Rust draws a line between safe and unsafe APIs. For a safe API, it is up to the API developer to design it such that it cannot be misused in a way that causes memory safety issues, and the only lifetime requirements it can impose are those that can be expressed in the Rust type system and statically checked at compile time. The DRM scheduler's complex chain of lifetime requirements cannot, and wrapping it in enough glue to remove those lifetime requirements would require about as much code as just rewriting it, as well as add unacceptable duplication and overhead.
In kernel Rust, we strive to only have safe APIs for components which have no fundamental reason for being unsafe (things like direct memory mapping and raw hardware access). The DRM scheduler does not fall into one of those "inherently unsafe" categories, so it doesn't make sense to provide a raw unsafe API for it. Doing so would just expose Rust code to the kind of subtle safety implications that cause memory problems every day in C. If I were to use drm_sched's unsafe API "as is" in my driver, it would *by far* be the least auditable, most complex usage of unsafe code in the entire driver, and I have no confidence that I would be able to get it right and keep it right as the driver changes.
I don't see a reason why this cannot be simply fixed in drm_sched, but Christian disagrees, and has repeatedly (and strongly) NAKed my attempts at doing so, and indeed NAKed the entire premise of the change in lifetime requirements itself. So here we are. There is no solution that will work for everyone that involves drm_sched.
So I don't have a choice other than to just not use drm_sched and roll my own. I will happily move that Rust implementation to common code if another Rust driver comes along and wants to use it. I'm not sure if that kind of thing can be easily made available to C drivers in reverse, but I guess we'll cross that bridge when a C driver expresses interest in using it.
So far it seems existing C drivers are happy with drm_sched's design and complex lifetime requirements, even though they aren't even documented. Perhaps they managed to reverse engineer them from the source, or someone told the authors about it (certainly nobody told me when I started using drm_sched). Or maybe a bunch of the drm_scheduler users are actually broken and have memory safety issues in corner cases. I don't know, though if I had to bet, I'd bet on the second option.
Actually, let's do a quick search and find out!panfrost_remove() -> panfrost_device_fini() -> panfrost_job_fini() -> drm_sched_fini()
There is a direct, synchronous path between device removal and destroying the DRM scheduler. At no point does it wait for any jobs to complete. That's what patch #3 in this series tries to fix.
In fact, it doesn't even keep the entities alive! It calls drm_dev_unregister() before everything else, but as the docs for the DRM driver lifetimes clearly say (see, docs!), objects visible to userspace must survive that and only be released from the release callback. drm_sched entities are created/destroyed from panfrost_job_open()/panfrost_job_close(), which are called from panfrost_open() and panfrost_postclose(), which are the userspace file open/close functions.
That one I fix in the Rust abstraction already (that one's relatively easy to fix), so it doesn't need a drm_sched patch from my point of view, but it is yet another subtle, undocumented lifetime requirement that is, evidently, impossible to know about and get right without documentation.
Meanwhile, panfrost_fence_ops has no remove() callback, which means there is no reference path stopping device removal (and therefore scheduler teardown) or even module unload while driver fences are still alive. That leads to the UAF patch #2 in this series tries to fix.
In other words, as far as I can tell, the panfrost driver gets *everything* wrong when it comes to the DRM scheduler lifetime requirements, and will crash and burn if the driver is unbound while jobs are in flight, anyone has a file descriptor open at all, or even if any shared buffer holding a DRM scheduler fence from it is bound to the display controller at that time.
This is why this kind of design is not allowed in Rust. Because nobody gets it right. *Especially* not without docs. I assumed, like the authors of the Panfrost driver clearly assumed, that the DRM scheduler API would not have these crazy undocumented hidden requirements. The only reason I found out the hard way is I happen to create and destroy schedulers all the time, not just once globally, so I would hit the bugs and dangling pointers much more often than Panfrost users who, most likely, never unbind their devices. But we both have the same problem.
I think I've done all I can to explain the issues and try to fix them, so the ball is in your court now. If you want to keep the current design, that's fine, but Rust code will not be a drm_sched user in that case. And the rest of the DRM folks in the C world will have to contend with these issues and fix all the problems in the C drivers (I'm sure panfrost isn't the only one, it's just literally the first one I looked at).
As for me, I'm happy to write a simple workqueue-based Rust scheduler suitable for firmware-managed scheduler devices. Honestly, at this point, I have very little faith left in my ability to fix all these issues in drm_sched (I know there's at least one more lurking, I saw a NULL deref but I wasn't able to reproduce it nor divine how it possibly happened). That, combined with the hostility from the AMD folks about my attempts to improve drm_sched even just a little bit, makes that decision very easy.
Farewell, DRM scheduler. It was nice trying to work with you, but things just didn't work out. I won't be submitting a v2 to this series myself. Please ping me if you fix all these fundamental design issues with drm_sched and think I might actually be able to use it safely in Rust one day. If the API design is solid and safe and the implementation done in a way that inspires confidence at that time maybe we can yank out my Rust solution when the time comes and switch back to drm_sched.
Just please don't expect me to do the work any more, I've done everything I can and this now has to come from you, not me. I've spent way more time understanding drm_sched, auditing its code, understanding its design intent, trying to fix it, and getting yelled at for it than it would take to write a new, clean, safe Rust scheduler. I don't regret some of the time spent (some of the implementation details of drm_sched have taught me useful things), but I'm not prepared to spend any more, sorry.
~~ Lina
