On 18/02/2021 18:20, Daniel Vetter wrote:
On Thu, Feb 18, 2021 at 6:16 PM Rob Herring <robh+dt@xxxxxxxxxx> wrote:
On Thu, Feb 18, 2021 at 10:51 AM Steven Price <steven.price@xxxxxxx> wrote:
On 18/02/2021 16:38, Rob Herring wrote:
On Thu, Feb 18, 2021 at 10:15 AM Steven Price <steven.price@xxxxxxx> wrote:
On 18/02/2021 15:45, Alyssa Rosenzweig wrote:
Yeah plus Cc: stable for backporting and I think an igt or similar for
panfrost to check this works correctly would be pretty good too. Since
if it took us over 1 year to notice this bug it's pretty clear that
normal testing doesn't catch this. So very likely we'll break this
again.
Unfortunately there are a lot of kernel bugs which are noticed during actual
use (but not CI runs), some of which have never been fixed. I do know
the shrinker impl is buggy for us, if this is the fix I'm very happy.
I doubt this will actually "fix" anything - if I understand correctly
then the sequence which is broken is:
* allocate BO, mmap to CPU
* madvise(DONTNEED)
* trigger purge
* try to access the BO memory
which is an invalid sequence for user space - the attempt to access
memory should cause a SIGSEGV. However because drm_vma_node_unmap() is
unable to find the mappings there may still be page table entries
present which would provide access to memory the kernel has freed. Which
is of course a big security hole and so this fix is needed.
In what way do you find the shrinker impl buggy? I'm aware there's some
dodgy locking (although I haven't worked out how to fix it) - but AFAICT
it's more deadlock territory rather than lacking in locks. Are there
correctness issues?
What's there was largely a result of getting lockdep happy.
btw for testing shrinkers recommended way is to have a debugfs file
that just force-shrinks everything. That way you avoid all the trouble
that tend to happen when you drive a system close to OOM on linux, and
it's also much faster.
2nding this as a good idea.
Sounds like a good idea to me too. But equally I'm wondering whether the
best (short term) solution is to actually disable the shrinker. I'm
somewhat surprised that nobody has got fed up with the "Purging xxx
bytes" message spam - which makes me think that most people are not
hitting memory pressure to trigger the shrinker.
If the shrinker is dodgy, then it's probably good to have the messages
to know if it ran.
The shrinker on kbase caused a lot of grief - and the only way I managed
to get that under control was by writing a static analysis tool for the
locking, and by upsetting people by enforcing the (rather dumb) rules of
the tool on the code base. I've been meaning to look at whether sparse
can do a similar check of locks.
Lockdep doesn't cover it?
Short answer: no ;)
It's pretty good actually, if you correctly annotate things up.
I agree - it's pretty good, the problem is you need reasonable test
coverage, and getting good test coverage of shrinkers is hard.
The problem with lockdep is that you have to trigger the locking
scenario to get a warning out of it. For example you obviously won't get
any warnings about the shrinker without triggering the shrinker (which
means memory pressure since we don't have the debugfs file to trigger it).
Actually, you don't need debugfs. Writing to /proc/sys/vm/drop_caches
will do it. Though maybe there's other code path scenarios that
wouldn't cover.
Huh didn't know, but it's a bit a shotgun, plus it doesn't use
fs_reclaim shrinker annotations, which means you don't have lockdep
checks. I think at least, would need some deadlock and testing.
The big problem with this sort of method for triggering the shrinkers is
that they are called without (many) locks held. Whereas it's entirely
possible for a shrinker to be called at (almost) any allocation in the
kernel.
Admittedly the Panfrost shrinkers are fairly safe - because most things
are xxx_trylock(). kbase avoids trylock which makes reclaim more
reliable, but means deadlocks are much easier.
I have to admit I'm not 100% sure I've seen any lockdep warnings based
on buffer objects recently. I can trigger them based on jobs:
[snip]
Certainly here the mutex causing the problem is the shrinker_lock!
The above is triggered by chucking a whole ton of jobs which
fault at the GPU.
Sadly I haven't found time to work out how to untangle the locks.
They are tricky because pretty much any memory allocation can trigger
things as I recall.
The above should only be possible with my dma_fence annotations, and
yes the point to bugs in the drm/scheduler. They shouldn't matter for
panfrost, and those patches aren't in upstream yet.
Yes that's on a (random version of) drm-misc - just what I happened to
have built recently. Good news if that's not actually Panfrost's bug. I
haven't had the time to track down what's going on yet.
Sounds like I'm perhaps being a bit unfair on the shrinkers - I'm just
aware that I went down a rabbit hole before looking at changing the
locks which started because I was convinced having shrinker_lock as a
mutex was a problem. But it was a while ago and I've forgotten what the
logic was.
Steve
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
