On Friday, July 24, 2020 5:19 PM, Paul Menzel <pmen...@molgen.mpg.de> wrote:
> Dear Kees, > > Am 24.07.20 um 19:33 schrieb Kees Cook: > > > On Fri, Jul 24, 2020 at 09:45:18AM +0200, Paul Menzel wrote: > > > > > Am 24.07.20 um 00:32 schrieb Kees Cook: > > > > > > > On Thu, Jul 23, 2020 at 09:10:15PM +0000, Mazin Rezk wrote: > > > > As Linux 5.8-rc7 is going to be released this Sunday, I wonder, if > > > > commit > > > > 3202fa62f ("slub: relocate freelist pointer to middle of object") > > > > should be > > > > reverted for now to fix the regression for the users according to > > > > Linux’ no > > > > regression policy. Once the AMDGPU/DRM driver issue is fixed, it can be > > > > reapplied. I know it’s not optimal, but as some testing is going to be > > > > involved for the fix, I’d argue it’s the best option for the users. > > > > Well, the SLUB defense was already released in v5.7, so I'm not sure it > > really helps for amdgpu_dm users seeing it there too. > > In my opinion, it would help, as the stable release could pick up the > revert, ones it’s in Linus’ master branch. > > > There was a fix to disable the async path for this driver that worked > > around the bug too, yes? That seems like a safer and more focused > > change that doesn't revert the SLUB defense for all users, and would > > actually provide a complete, I think, workaround whereas reverting > > the SLUB change means the race still exists. For example, it would be > > hit with slab poisoning, etc. > > I do not know. If there is such a fix, that would be great. But if you > do not know, how should a normal user? ;-) > > Kind regards, > > Paul > > Kind regards, > > Paul If we're talking about workarounds now, I suggest simply swapping the base and context variables in struct dm_atomic_state. By that way, we won't need to change non-amdgpu parts of the code (e.g. by reverting the SLUB patch). Prior to 3202fa62f, the freelist pointer was stored in dm_state->base which was never dereferenced and therefore caused no noticeable issue. After 3202fa62f, the freelist pointer is stored in the middle of the struct (i.e. dm_state->context). Swapping the position of the base and context variables in dm_atomic_state should, in theory, revert this code back to it's pre-5.7 state since the code would be back to overwriting base instead. If we decide to use this workaround, I can write the patch and do more extended tests to confirm it works around the issues. That said, I haven't seen the async disabling patch. If you could link to it, I'd be glad to test it out and perhaps we can use that instead. Thanks, Mazin Rezk