On Tue, Dec 05, 2017 at 08:43:49AM -0800, James Bottomley wrote: > On Wed, 2017-12-06 at 00:38 +0800, Ming Lei wrote: > > On Tue, Dec 05, 2017 at 04:22:33PM +0000, Bart Van Assche wrote: > > > > > > On Tue, 2017-12-05 at 13:00 +0800, Ming Lei wrote: > > > > > > > > No, do not mix two different things in one patch, especially the > > > > fix part need to be backported to stable. > > > > > > > > The fix part should aim at V4.15, and the other part can be a > > > > V4.16 stuff. > > > > > > Does this mean that you do not plan to post a v5 of your patch and > > > that you want me to rework this patch series? I can do that. > > > > I believe V4 has been OK for merge, actually the only concern from > > James is that 'set the cmnd to NULL *before* calling free so we > > narrow the race window.', but that isn't required as I explained, > > even though you don't do that in this patch too. > > > > https://marc.info/?t=151030464300003&r=1&w=2 > > > > I will work on V5 if Martin/James thinks it is needed. > > I don't buy that it isn't needed. The point (and the pattern) is for a > destructive action set the signal *before* you execute the action not > after. The reason should be obvious: if you set it after you invite a > race where the check says OK but the object has gone. Even if the race Even you do that, the race is still highly likely there: 1) mempool_free() can be much quicker than scsi_show_rq() because it is a local free, and scsi_show_rq() can be run from remote CPU wrt. the allocated 'cmd->cmnd', and access to remote NUMA node should be slower than mempool_free(), so use-after-free is triggered. 2) any preemption or local IRQ in scsi_show_rq() can make it touch a freed buffer, and sd_uninit_command() is run from irq context. 3) no any barrier is applied, so the actual write can be reordered in sd_uninit_command() So setting the cmd->cmnd as NULL before mempool_free() can't avoid the use-after-free, scsi_show_rq() has to survive that, then do we really need to add the unnecessary change in sd_uninit_command()? Not mention the change will make the debug info disappear too early, is that what we need? > is highly unlikely, the pattern point still holds. -- Ming
