On 24/07/2020 17:12, Jens Axboe wrote: > On 7/24/20 6:52 AM, Pavel Begunkov wrote: >> On 24/07/2020 15:46, Pavel Begunkov wrote: >>> On 24/07/2020 01:24, Jens Axboe wrote: >>>> On 7/23/20 4:16 PM, Jens Axboe wrote: >>>>> On 7/23/20 12:12 PM, Pavel Begunkov wrote: >>>>>> poll_add can have req->work initialised, which will be overwritten in >>>>>> __io_arm_poll_handler() because of the union. Luckily, hash_node is >>>>>> zeroed in the end, so the damage is limited to lost put for work.creds, >>>>>> and probably corrupted work.list. >>>>>> >>>>>> That's the easiest and really dirty fix, which rearranges members in the >>>>>> union, arm_poll*() modifies and zeroes only work.files and work.mm, >>>>>> which are never taken for poll add. >>>>>> note: io_kiocb is exactly 4 cachelines now. >>>>> >>>>> I don't think there's a way around moving task_work out, just like it >>> >>> +hash_node. I was thinking to do apoll alloc+memcpy as for rw, but this >>> one is ugly. >>> >>>>> was done on 5.9. The problem is that we could put the environment bits >>>>> before doing task_work_add(), but we might need them if the subsequent >>>>> queue ends up having to go async. So there's really no know when we can >>>>> put them, outside of when the request finishes. Hence, we are kind of >>>>> SOL here. >>>> >>>> Actually, if we do go async, then we can just grab the environment >>>> again. We're in the same task at that point. So maybe it'd be better to >>>> work on ensuring that the request is either in the valid work state, or >>>> empty work if using task_work. >>>> >>>> Only potential complication with that is doing io_req_work_drop_env() >>>> from the waitqueue handler, at least the ->needs_fs part won't like that >>>> too much. >>> >>> Considering that work->list is removed before executing io_wq_work, it >>> should work. And if done only for poll_add, which needs nothing and ends up >>> with creds, there shouldn't be any problems. I'll try this out >> >> Except for custom ->creds assigned at the beginning with the personality >> feature. Does poll ever use it? > > It's kind of annoying how we don't have a def->needs_creds, because lots > of things would never use it. For poll, it wouldn't be used at all, > which makes this issue doubly annoying. Then we don't have to care which one it has, and the scheme should work good enough for a quick fix. I still don't like overwriting work.list until it leaves io-wq, but that's to think about for 5.9 -- Pavel Begunkov
