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:

quoted

On Tue, Dec 05, 2017 at 04:22:33PM +0000, Bart Van Assche wrote:

quoted

On Tue, 2017-12-05 at 13:00 +0800, Ming Lei wrote:

quoted

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