Hi, welcome to my talk. My name is Bartosz Gowaszewski. I work for Linaro. And today
I wanted to talk about an issue that had been bothering me for some time, namely a certain
family of youth after free bucks in the kernel that has been, for a long time, blamed on
Devres and then that I decided to investigate a bit and only to find out that reality is
often disappointing and everything is broken and the problem is actually much worse than
if that was Devres' fault. So yeah, without further ado, let's stack in because the time
is short. So typically your typical probe function in a device driver would look something
like this without Devres. So you allocate some resource. If it fails, you bail out. You
allocate a second resource. If it fails, you free the previous one, bail out, and so on
and so forth. So every next resource would need, if it fails, you need to free the previous
ones. Alternatively, you would do something like this. You would have labels at the end
of the function and just jump to it when it's one of the allocations or resources. You wouldn't
be able to acquire one of the resources. You would jump to it and free every previously
allocated resource in reverse order. And then you need your remove function that would
also drop these resources in reverse order. But you can use Devres and in this case it
looks much better. You allocate a resource. If it fails, you bail out. You allocate a
second resource. If it fails, you bail out immediately. You don't have to free those
resources yourself. When a driver is detached, they will be dropped automatically in reverse
order and with that you no longer need your remove function. So that is pretty sweet.
But you will notice, if you send enough patches to different subsystems, you will notice that
certain maintainers show a version to Devres. You would get comments like, oh no, keep Devres
out of my subsystem or don't you know, Devres leads to, using these interfaces leads to
crashes. But without, you would probably not get a lot of explanation on what the problem
is. And so last year I stumbled upon a talk by Laurent Panchard from last year's Linux
Plumbers conference. It's actually, the link is below so you can watch it. And yes, I watched
that and then I started browsing Lore and noticed that Laurent actually had sent an
email about that already in 2015 saying that if you open a device node in user space and
you unbind the driver that exposed it, exported it, and try to do something with that open
file descriptor called one of the system calls, you will get a nice stack trace in your kernel
log. So I thought, okay, that's interesting. I read throughout discussion. I found out
that there were several such discussions previously, of which I had not been aware.
And I don't want to go into much detail, but the gist of it is that there are certain users
in the kernel, mostly device drivers, or only device drivers, that allocate using devres,
like case alloc in this example, allocate memory that is dropped in remove, but that's
actually still referenced elsewhere, which leads to use after freebacks. And the first
thing that popped in my mind was how is that different from calling any, from dropping these
resources in remove. And I wasn't the only one, because many people asked about it. And
one of the responses when something like this, it's not, it's the same thing, but the problem
is that people don't understand the lifetime rules and expect magic interfaces to fix it
for them. So I thought that this, while this is certainly true, I think that driver developers
should not be expected to understand every detail of how kernel subsystems work, because
they actually should care more about making the device work. And I would argue that you
should actually expect magic interfaces in drivers to fix stuff for you. And if it's
not straightforward, then it's a problem of the subsystem or of given interface and
not of device driver developers. So I decided to investigate and see if maybe something
changed. And I do maintain the GPIO subsystem. So this was the first one that I tried. I
had this serial to USB converter cable plugged in that actually also has four GPIO pins.
So I opened the GPIO device. I unplugged the cable. I tried to read the value of the pin
and sure enough, it crashed, no point under reference. I was like, okay, that's interesting.
I did not expect that, but it's certainly interesting. So I tried a different subsystem.
I tried I2C. Surprisingly, it didn't crash. It just, like, when I tried to unbind the
driver, it just hang, froze and did nothing. Okay, so that's interesting. But I realized
that when I unplugged the cable, I had my picocon console open and it just, it didn't
crash. It just exited. So certainly, UART is doing something right and other subsystems
are not doing it correctly. So I decided to investigate a bit. So in the GPIO subsystem,
we have two types of structures. One that the drivers see. This is the GPIO chip. And
the second one that the drivers, the GPIO providers don't touch, don't even have access
to, which is the GPIO device. And so this will be important in a second. So I looked
at the crash that I had in GPIO and it turned out that it was a no pointer to reference
at a certain line, the one that you can see above. And at the moment where I, like, I
still had my device open, I tried to call one of the system calls and it turned out
that the chip that you can see above, when the referencing gdev was no, was already gone.
So this is nothing to do with DeVres. It's just that we had a bug in the GPIO character
device code where when we, when the driver is going away and it calls GPIO chip remove,
it can be called from DeVres or it can be called in your remove function. We simply
said the chip to null because the driver is gone, but the, the, the, the, the, the, the
struct device still can be referenced elsewhere. So we just numbed it down, which is the correct
thing to do, but it, it still, it still needs to be, needs to be checked in the, in the
character device code. This is, this was what we were not doing and this needs fixing clearly.
And there is also a question of a race condition in the, in the, in the system call callbacks
where if, even if we do check it in the beginning, we still need some locking because otherwise
the, the driver can be removed when we are still executing the system call. So I looked
at that and I thought, okay, this is easy enough to fix and that, that's definitely
not, not linked to those errors that the discussion in, in the email thread was about. So I went
over to I2C. I decided to see what's, what's going on in I2C. Why, why does it, why it
can't I unbind the driver as for as long as I keep the device open? And it turned out
that there is this strange completion and a comment about it, making it so that when
the driver, when the driver is trying to delete the I2C adapter, it waits for as long as there
are still references to the underlying struct device. Okay. It's, it's, it's not definitely
the, the, the, the, the freezing when, when you're trying to unbind the driver is not
the correct way, but at least it doesn't crash and someone had something, there was a purpose
to, to doing that. So I, I thought, okay, so why, why does UART work? I, I looked at
the UART code and figured out that actually UART does a smart thing. First, when you go
into any of the, of the system call callbacks in the kernel, you check, you have a similar
split like GPIO when you have a, where you have a structure that lives for as long as
the struct device lives and a separate structure that is allocated by the driver. So it turned
out that, yeah, it, it first checks if the driver is still there. If it is, then it locks
it so that it cannot go away from under an executing system call. Okay. So this definitely
makes sense. And I also noticed that spy def works fine, but upon further instruction,
inspection of the code, it turns out that it also suffers from a race condition because
the, when the spy def spy is checked, the spin lock is only taken for the duration of
the check or like for, for reading that, for reading of the pointer, but later it's still
that drive, the underlying driver can still go away while the lock is already released.
So this is a concurrency issue. So I started thinking that there is, there is some misconception
about Devers. I decided to fix some things. I started with GPIO. I sent some patches.
They, they were, they, they went into, into mainline. They, they seemed like in this case,
they did fix the issues. The, the, the user space would no longer crash. The kernel would
no longer crash when the user space would unbind the driver and use the character device.
I also sent some fixes to, for spy def. And then I send a fix for ITC. I remove this completion.
This is when, when things went sideways actually. I, I removed that completion. I added locking.
I started fiddling with this character device and I was proud of myself because I, I thought
that I fixed this problem that has for, for a long time existed in ITC. And Wolfram, the
maintainer of ITC, took that patch, reviewed it, said it passes all the, all his stress
testing, but he's, he's having a gut feeling that something is wrong. And after a couple
of days, he sends me an email and says that, okay, I found this discussion from a couple
of years ago where this was explained in detail. And what happened? So what, what turned out,
turned out to be the, the case with ITC. It turns out that ITC is a subsystem where drivers
allocate the ITC, the struct ITC adapter, which embeds struct device. And they allocate
that structure as part of a usually bigger structure that contains driver specific fields
in probe. And then on remove, in the remove callback, they drop that memory, but it contains
struct device, which is reference counted, unlike the, the, the structure that embeds
it. So this is, this is why this, this whole completion waiting for completion is, is there
in ITC because you must not drop free this memory containing struct device for as long
as there are references to struct device. And I noticed that this is not the only subsystem
that does it. There are, so every driver subsystem does things a bit differently. Some of them
have that split, some of them don't. For instance, spy has the same problem as ITC, but unlike
ITC, it doesn't expect the driver to allocate that data as part of the driver data. It expects
it to be allocated separately and hand it over to, to the spy subsystem, which is not
probably, it could use some improvement, but at least it doesn't crash and doesn't require
the same type of weight, synchronous waiting for, for, for dropping all the, all the references
to struct device. So actually this, this talk should be called don't let drivers allocate
and control the lifetime of struct device because this is the, the, the culprit basically.
So we, we have those systems that allocate struct device and there are more references
to it. It's still referenced elsewhere. And then we drop this memory and we still, when
the reference is like the, the, the, the, the driver along the, the struct device no
longer exists, but it's, it's still referenced somewhere. And then the subsystem, the driver
model tries to call, for instance, the release callback of the device and there's nothing
there. So we, we have those crashes. So I, I, I didn't look at all the subsystems clearly
and there are too many, but I, I just noticed that certain parts of the kernel get it right.
So GPIO now with those fixes is, is fine. You are just fine. Word.doc is fine. They,
they have this split where the struct device is allocated and managed by the subsystem
while driver data does not contain the struct device. I'm not talking about the struct device
that is passed to probe. I'm talking about struct device that is allocated by the drivers
or the, the, the respective subsystems for those proper underlying devices. So like we
have this, let's say a platform device for a GPIO chip and then we allocate a struct
device per every bank. Just an example and many subsystems do that too. So there, there's
this problem, but there are also other problems. So even those subsystems that get this part
right often suffer from concurrency issues because there is no locking in the system
call, callbacks in the kernel. So even if they do check if the, if the driver is still
there or the device is still there or attached to the driver, they often don't lock the state.
So it's, it's, it's possible that the driver will go away one, while they're still executing
and they're referencing that pointer. This was the case in, in spy for instance. And
I think that the issue is just about the logical scope of objects and not the scope
in understood as, as, as a scope of a variable in a C programming language, but more, more
like a logical scope of objects where you have, if something is allocated in probe,
when you, when you attach, first attach the device to the driver and, and you allocate
something in probe, it should only need to exist for as long as the driver is attached
and as, as soon as it goes away, it should be freed and removed, be it in, with dev res
or, or, or in remove. And yeah, so, so there is this problem with, with many subsystems
that they don't have this and they let drivers allocate some data in probe and then handle
it over to the subsystem or, or even do it implicitly, which leads to all these errors.
So I, I know that Laurent's area of interest will probably be media and DRM, so I just
skimmed through, through the use, user space device notes, codes in, in those subsystems
and noticed that they seem to be getting part of it correctly, but there are concurrency
issues as well. And the problem with DRM is that the RAM, the, the device handling code,
the, the, the, the character block device, the, the handling code is not, is, is not
centralized within the files, within the subsystem, meaning that we have many different struct
file operations with different implementations for different callbacks, correct me if I'm
wrong afterwards, but it, it, it seemed like, like it from, I'm not an expert on DRM, but
it seemed like it just from looking at it. So what about dev res? Is it safe? I have
found no evidence that it isn't. And if something can be freed and removed, it can be freed
with dev res because dev res will do just that. It will, as soon as the driver gets
detached from the device, the other way around, it will, it will free all, all resources allocated
with dev res in reverse order. There is, there are some, some, some issues like dev mk realloc
could use some semantic clarification because as, as it is right now, it's not clear whether
the order, if you call dev mk realloc, does the order change or not when, when releasing
those resources. In any case, I, is my strong belief that dev res makes code much more readable,
safer, and it actually should be encouraged and, and not discouraged, but it has a limited
scope. And on that point, how can we supplement it? Because a certain semblance of resource,
of automated resource management, RAII, if you will, in C would be useful in the kernel.
So yeah, the first thing that comes to mind would be using Rust. With Rust, these situations
that I described would never, would clearly never, never be allowed to happen. But that's
not, for, for now, we're still coding in C. So I was thinking that if you've ever coded
in, in, in a user space library like Gillib, for instance, which is, I, I, I believe a golden
standard of C programming in, in user space, you would notice that they use, make a, use
of a lot of, clean up, clean up attributes. And this is something that GCC and Clang support,
and I haven't seen that in the kernel, and I'm wondering why it's, because if we would
use reference counting in conjunction with cleanup, we could actually make it.
If I can interject for one second, but actually, at least not in core kernel code, but Peter
Zilstra used this in somewhere in the kernel source code tree, at least. And I had proposed
something like this, at least in person to a couple of people before, because I want
to make use of this as well. So I would be very happy if this happened.
Yeah. So I have a small example. So if you are, if you are not familiar with the cleanup
attribute, it allows you to specify a cleanup function for a variable. And when the variable
goes out of scope, it's called, so it's like a destructor in C++ basically. Alone, it's,
it's useful within the scope of a single code block or a single function, but in conjunction,
well, this is just another example of how to, how to use it, but in conjunction with reference
counting, it's, it's quite useful because you can, like, you can see the, the, the foo
and bar, foo create and bar functions on the right. You allocate a, a reference counted,
automatically reference counted resource. You do something with it. If you bail out, the
reference count goes to zero and then it's freed. But if you return it while also increasing
the ref count and then grab it in another function in bar in this, in this case, then
as soon as the, the, the foo create function returns, the reference count is decreased,
but it's already two. So it goes down to one and we, without having to control free those
resources manually, we just keep track of the references just by using reference counting
and cleanup. And this is what Gillib does a lot. And it, and it works quite nice. It's,
it makes programming in C in this space much easier. And what to do about the offending
subsystems? It's a case by case issue because every subsystem does it differently. And I
tried, I, I spoke to Wolfram a bit about what can we do in I2C. And it's, it would be very
hard because you would have to do a sweeping change across the entire subsystem to make
drivers not allocate I2C adapter, not, not I2C adapter, but rather the underlying struct
device on its own instead let it handle, let, let I2C, the I2C subsystem handle it. Yes,
I just wanted to bring that up and that's, that's it. I'm right on time. All right, questions?
Thank you. I, I, I like your second slide. I felt when you say I was not completely wrong
with my talk, so thank you for that. I was a bit worried. A few comments. It's multiple
problems actually that you're trying to solve here. One is the race condition between IOC
TL, so use space access in general and the remove function. And for everything that's
based on character devices, there's a patch from Dan Williams that was posted in 2021.
I think I don't know if you've seen that one, that attempts to fix it at the C dev level,
which I think is the right place to do it instead of duplicating the same fix in all
the subsystems. So it has been positively reviewed, not merged. I think it was just
one review command that said that debug FS and proc FS had similar constructs. And so
he was asked to just refactor the code to reuse the same instead of duplicating it.
But that should be something we could upstream and solve. So that's one of them. This, I
agree with you that there's nothing wrong with dev res or managed API in general. It
was mostly DevM case at the lock in particular that I had trouble with. Things like DevM,
IO remap for instance, is perfectly fine because you tie the lifetime management of the resource
with the physical strike device, which is removed at the end of the remove function,
and that is what you should do. So that's totally fine. The issues with the DevM functions
come when we try to tie, as you said, the lifetime of a resource to the wrong device.
DRM has grown its custom managed helpers based on dev res. So not DevM, but it's DRMM. That
does do it relatively right. They tie the memory management to the CDEV that's exposed
to user space. But where it breaks is when you have one physical device that exposed
multiple CDEVs because then you have a top level data structure that covers all of those.
So even if you allocate each of them dynamically, you will need to make sure that the top level
structure will be released only when nothing else can have a reference to it. So I think
there will always be cases where reference counting will be needed and the drivers will
have to handle that. But in many cases, helpers should be possible to make it simpler.
And also I mentioned that on the slide that I didn't say it out loud. You have some diverse
resource, the diverse helpers that reach into other subsystems, which also have their own
issues. So this can be dangerous as well.
So I have seen in multiple places where drivers are allocating the strike device that is then
getting passed to the framework. But then the solution that they are using is that they
are basically instead of freeing the device in remove, they are just dropping the reference
to that device that has been allocated. So someone is taking the ownership of that device
and then you rely on the reference counting to that device and then freeing that strike
device only from the release callback that is then set by the driver itself.
This is what SPY does. But I2C is a worse example because you usually have a driver data.
Inside it you have I2C adapter. Inside it you have strike device. And this strike device
has this release callback. But in the release callback, you are in the subsystem, it cannot
know what the outer layer structure is and where is it, what offset it adds to free it.
So it sets the release callback of the strike device. It's another workaround I guess.
It seems to me that the right thing to do is not let drivers allocate strike device instead
do it in the subsystem if you need to. This also hides some additional complexity from
the driver developers.
On the slide about the state of the current subsystems you had a question mark after SPY.
I wonder if you have doubts if it's fine today.
Yeah, I think, no, this is the one, right?
That one.
This one?
Yeah, there is a question mark in the SPY.
Yes, I put it there because I haven't investigated that in detail so I wasn't sure if it works
fine. It looks to be working fine, like just from testing it. I wasn't sure if it's correctly
implemented, let's say, because it sounds good in theory.
All right, the last question.
So for systems like I2C, has anybody calculated the amount of pain for things like the I2C,
has anybody calculated the amount of actual work to fix this?
It looks like it's a lot because I did send some proposition to Wolfram about wrapping
every the reference of the I2C adapter that dev into a helper that would then allow us
to change that strike device into a pointer instead of the proper structure and allocate
that inside the subsystem but it's not going to be easy.
This is why I dropped the patch. I told Wolfram not to pursue that because it looks to me
like it's not going to be that easy and I don't really want to get my hands that dirty.
So for somebody else then?
It's been like this for a long time so I'm not sure if anyone's going to step up to try
to fix that.
Okay, thank you.
All right, thanks.
Thank you.