diff mbox series

scsi: target: tcmu: fix size in calls to tcmu_flush_dcache_range

Message ID 20200528193108.9085-1-bstroesser@ts.fujitsu.com
State New
Headers show
Series scsi: target: tcmu: fix size in calls to tcmu_flush_dcache_range | expand

Commit Message

Bodo Stroesser May 28, 2020, 7:31 p.m. UTC
1) If remaining ring space before the end of the ring is
   smaller then the next cmd to write, tcmu writes a padding
   entry which fills the remaining space at the end of the
   ring.
   Then tcmu calls tcmu_flush_dcache_range() with the size
   of struct tcmu_cmd_entry as data length to flush.
   If the space filled by the padding was smaller then
   tcmu_cmd_entry, tcmu_flush_dcache_range() is called for
   an address range reaching behind the end of the vmalloc'ed
   ring.
   tcmu_flush_dcache_range() in a loop calls
      flush_dcache_page(virt_to_page(start));
   for every page being part of the range. On x86 the line is
   optimized out by the compiler, as flush_dcache_page() is
   empty on x86.
   But I assume the above can cause trouble on other
   architectures that really have a flush_dcache_page().
   For paddings only the header part of an entry is relevant
   Due to alignment rules the header always fits in the
   remaining space, if padding is needed.
   So tcmu_flush_dcache_range() can safely be called with
   sizeof(entry->hdr) as the length here.

2) After it has written a command to cmd ring, tcmu calls
   tcmu_flush_dcache_range() using the size of a struct
   tcmu_cmd_entry as data length to flush.
   But if a command needs many iovecs, the real size of the
   command may be bigger then tcmu_cmd_entry, so a part of
   the written command is not flushed then.

Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
---
 drivers/target/target_core_user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Bodo Stroesser Aug. 28, 2020, 9:53 a.m. UTC | #1
Hi,

I'm adding stable@vger.kernel.org

On 2020-06-03 04:31, Martin K. Petersen wrote:
> On Thu, 28 May 2020 21:31:08 +0200, Bodo Stroesser wrote:

> 

>> 1) If remaining ring space before the end of the ring is

>>     smaller then the next cmd to write, tcmu writes a padding

>>     entry which fills the remaining space at the end of the

>>     ring.

>>     Then tcmu calls tcmu_flush_dcache_range() with the size

>>     of struct tcmu_cmd_entry as data length to flush.

>>     If the space filled by the padding was smaller then

>>     tcmu_cmd_entry, tcmu_flush_dcache_range() is called for

>>     an address range reaching behind the end of the vmalloc'ed

>>     ring.

>>     tcmu_flush_dcache_range() in a loop calls

>>        flush_dcache_page(virt_to_page(start));

>>     for every page being part of the range. On x86 the line is

>>     optimized out by the compiler, as flush_dcache_page() is

>>     empty on x86.

>>     But I assume the above can cause trouble on other

>>     architectures that really have a flush_dcache_page().

>>     For paddings only the header part of an entry is relevant

>>     Due to alignment rules the header always fits in the

>>     remaining space, if padding is needed.

>>     So tcmu_flush_dcache_range() can safely be called with

>>     sizeof(entry->hdr) as the length here.

>>

>> [...]

> 

> Applied to 5.8/scsi-queue, thanks!

> 

> [1/1] scsi: target: tcmu: Fix size in calls to tcmu_flush_dcache_range

>        https://git.kernel.org/mkp/scsi/c/8c4e0f212398

> 


The full commit of this patch is:
    8c4e0f212398cdd1eb4310a5981d06a723cdd24f

This patch is the first of four patches that are necessary to run tcmu
on ARM without crash. For details please see
    https://bugzilla.kernel.org/show_bug.cgi?id=208045
Upsteam commits of patches 2,3, and 4 are:
  2: 3c58f737231e "scsi: target: tcmu: Optimize use of flush_dcache_page"
  3: 3145550a7f8b "scsi: target: tcmu: Fix crash in tcmu_flush_dcache_range on ARM"
  4: 5a0c256d96f0 "scsi: target: tcmu: Fix crash on ARM during cmd completion"

Since patches 3 and 4 already were accepted for 5.8, 5.4, and 4.19, and
I sent a request to add patch 2 about 1 hour ago, please consider adding
this patch to 5.4 and 4.19, because without it tcmu on ARM will still
crash.

Thank you,
Bodo
Bodo Stroesser Aug. 28, 2020, 10:03 a.m. UTC | #2
Hi,
I'm adding stable@vger.kernel.org

Once again, this time really adding stable.

On 2020-06-03 04:31, Martin K. Petersen wrote:
> On Thu, 28 May 2020 21:31:08 +0200, Bodo Stroesser wrote:

>

>> 1) If remaining ring space before the end of the ring is

>>      smaller then the next cmd to write, tcmu writes a padding

>>      entry which fills the remaining space at the end of the

>>      ring.

>>      Then tcmu calls tcmu_flush_dcache_range() with the size

>>      of struct tcmu_cmd_entry as data length to flush.

>>      If the space filled by the padding was smaller then

>>      tcmu_cmd_entry, tcmu_flush_dcache_range() is called for

>>      an address range reaching behind the end of the vmalloc'ed

>>      ring.

>>      tcmu_flush_dcache_range() in a loop calls

>>         flush_dcache_page(virt_to_page(start));

>>      for every page being part of the range. On x86 the line is

>>      optimized out by the compiler, as flush_dcache_page() is

>>      empty on x86.

>>      But I assume the above can cause trouble on other

>>      architectures that really have a flush_dcache_page().

>>      For paddings only the header part of an entry is relevant

>>      Due to alignment rules the header always fits in the

>>      remaining space, if padding is needed.

>>      So tcmu_flush_dcache_range() can safely be called with

>>      sizeof(entry->hdr) as the length here.

>>

>> [...]

>

> Applied to 5.8/scsi-queue, thanks!

>

> [1/1] scsi: target: tcmu: Fix size in calls to tcmu_flush_dcache_range

>         https://git.kernel.org/mkp/scsi/c/8c4e0f212398

>


The full commit of this patch is:
      8c4e0f212398cdd1eb4310a5981d06a723cdd24f

This patch is the first of four patches that are necessary to run tcmu
on ARM without crash. For details please see
      https://bugzilla.kernel.org/show_bug.cgi?id=208045
Upsteam commits of patches 2,3, and 4 are:
    2: 3c58f737231e "scsi: target: tcmu: Optimize use of flush_dcache_page"
    3: 3145550a7f8b "scsi: target: tcmu: Fix crash in 
tcmu_flush_dcache_range on ARM"
    4: 5a0c256d96f0 "scsi: target: tcmu: Fix crash on ARM during cmd 
completion"

Since patches 3 and 4 already were accepted for 5.8, 5.4, and 4.19, and
I sent a request to add patch 2 about 1 hour ago, please consider adding
this patch to 5.4 and 4.19, because without it tcmu on ARM will still
crash.

Thank you,
Bodo
Greg KH Sept. 1, 2020, 2:02 p.m. UTC | #3
On Fri, Aug 28, 2020 at 12:03:38PM +0200, Bodo Stroesser wrote:
> Hi,
> I'm adding stable@vger.kernel.org
> 
> Once again, this time really adding stable.
> 
> On 2020-06-03 04:31, Martin K. Petersen wrote:
> > On Thu, 28 May 2020 21:31:08 +0200, Bodo Stroesser wrote:
> > 
> > > 1) If remaining ring space before the end of the ring is
> > >      smaller then the next cmd to write, tcmu writes a padding
> > >      entry which fills the remaining space at the end of the
> > >      ring.
> > >      Then tcmu calls tcmu_flush_dcache_range() with the size
> > >      of struct tcmu_cmd_entry as data length to flush.
> > >      If the space filled by the padding was smaller then
> > >      tcmu_cmd_entry, tcmu_flush_dcache_range() is called for
> > >      an address range reaching behind the end of the vmalloc'ed
> > >      ring.
> > >      tcmu_flush_dcache_range() in a loop calls
> > >         flush_dcache_page(virt_to_page(start));
> > >      for every page being part of the range. On x86 the line is
> > >      optimized out by the compiler, as flush_dcache_page() is
> > >      empty on x86.
> > >      But I assume the above can cause trouble on other
> > >      architectures that really have a flush_dcache_page().
> > >      For paddings only the header part of an entry is relevant
> > >      Due to alignment rules the header always fits in the
> > >      remaining space, if padding is needed.
> > >      So tcmu_flush_dcache_range() can safely be called with
> > >      sizeof(entry->hdr) as the length here.
> > > 
> > > [...]
> > 
> > Applied to 5.8/scsi-queue, thanks!
> > 
> > [1/1] scsi: target: tcmu: Fix size in calls to tcmu_flush_dcache_range
> >         https://git.kernel.org/mkp/scsi/c/8c4e0f212398
> > 
> 
> The full commit of this patch is:
>      8c4e0f212398cdd1eb4310a5981d06a723cdd24f
> 
> This patch is the first of four patches that are necessary to run tcmu
> on ARM without crash. For details please see
>      https://bugzilla.kernel.org/show_bug.cgi?id=208045
> Upsteam commits of patches 2,3, and 4 are:
>    2: 3c58f737231e "scsi: target: tcmu: Optimize use of flush_dcache_page"
>    3: 3145550a7f8b "scsi: target: tcmu: Fix crash in tcmu_flush_dcache_range
> on ARM"
>    4: 5a0c256d96f0 "scsi: target: tcmu: Fix crash on ARM during cmd
> completion"
> 
> Since patches 3 and 4 already were accepted for 5.8, 5.4, and 4.19, and
> I sent a request to add patch 2 about 1 hour ago, please consider adding
> this patch to 5.4 and 4.19, because without it tcmu on ARM will still
> crash.

I don't see such a request, and am confused now.

What exact commits do you want backported, and to what trees?

thanks,

greg k-h
Bodo Stroesser Sept. 1, 2020, 3:58 p.m. UTC | #4
On 2020-09-01 16:02, Greg KH wrote:
> On Fri, Aug 28, 2020 at 12:03:38PM +0200, Bodo Stroesser wrote:

>> Hi,

>> I'm adding stable@vger.kernel.org

>>

>> Once again, this time really adding stable.

>>

>> On 2020-06-03 04:31, Martin K. Petersen wrote:

>>> On Thu, 28 May 2020 21:31:08 +0200, Bodo Stroesser wrote:

>>>

>>>> 1) If remaining ring space before the end of the ring is

>>>>       smaller then the next cmd to write, tcmu writes a padding

>>>>       entry which fills the remaining space at the end of the

>>>>       ring.

>>>>       Then tcmu calls tcmu_flush_dcache_range() with the size

>>>>       of struct tcmu_cmd_entry as data length to flush.

>>>>       If the space filled by the padding was smaller then

>>>>       tcmu_cmd_entry, tcmu_flush_dcache_range() is called for

>>>>       an address range reaching behind the end of the vmalloc'ed

>>>>       ring.

>>>>       tcmu_flush_dcache_range() in a loop calls

>>>>          flush_dcache_page(virt_to_page(start));

>>>>       for every page being part of the range. On x86 the line is

>>>>       optimized out by the compiler, as flush_dcache_page() is

>>>>       empty on x86.

>>>>       But I assume the above can cause trouble on other

>>>>       architectures that really have a flush_dcache_page().

>>>>       For paddings only the header part of an entry is relevant

>>>>       Due to alignment rules the header always fits in the

>>>>       remaining space, if padding is needed.

>>>>       So tcmu_flush_dcache_range() can safely be called with

>>>>       sizeof(entry->hdr) as the length here.

>>>>

>>>> [...]

>>>

>>> Applied to 5.8/scsi-queue, thanks!

>>>

>>> [1/1] scsi: target: tcmu: Fix size in calls to tcmu_flush_dcache_range

>>>          https://git.kernel.org/mkp/scsi/c/8c4e0f212398

>>>

>>

>> The full commit of this patch is:

>>       8c4e0f212398cdd1eb4310a5981d06a723cdd24f

>>

>> This patch is the first of four patches that are necessary to run tcmu

>> on ARM without crash. For details please see

>>       https://bugzilla.kernel.org/show_bug.cgi?id=208045

>> Upsteam commits of patches 2,3, and 4 are:

>>     2: 3c58f737231e "scsi: target: tcmu: Optimize use of flush_dcache_page"

>>     3: 3145550a7f8b "scsi: target: tcmu: Fix crash in tcmu_flush_dcache_range

>> on ARM"

>>     4: 5a0c256d96f0 "scsi: target: tcmu: Fix crash on ARM during cmd

>> completion"

>>

>> Since patches 3 and 4 already were accepted for 5.8, 5.4, and 4.19, and

>> I sent a request to add patch 2 about 1 hour ago, please consider adding

>> this patch to 5.4 and 4.19, because without it tcmu on ARM will still

>> crash.

> 

> I don't see such a request, and am confused now.

> 

> What exact commits do you want backported, and to what trees?

> 

> thanks,

> 

> greg k-h

> 


Sorry for the confusion.

The subject of the request I mentioned is
    "Re: [PATCH v2 0/2] scsi: target: tcmu: fix crashes on ARM"
because it is for the first patch of a small series of two.

Please backport to kernels 4.19 and 5.4 (it is part of 5.8 from beginning):
  8c4e0f212398 "scsi: target: tcmu: fix size in calls to tcmu_flush_dcache_range"

Please backport to kernels 4.19, 5.4 and 5.8:
  3c58f737231e "scsi: target: tcmu: Optimize use of flush_dcache_page"

Backporting to 4.14 or earlier AFAICS would need more work, especially testing.
I don't think that its worth it.

Thank you,
Bodo
Greg KH Sept. 4, 2020, 11:55 a.m. UTC | #5
On Tue, Sep 01, 2020 at 05:58:29PM +0200, Bodo Stroesser wrote:
> On 2020-09-01 16:02, Greg KH wrote:

> > On Fri, Aug 28, 2020 at 12:03:38PM +0200, Bodo Stroesser wrote:

> >> Hi,

> >> I'm adding stable@vger.kernel.org

> >>

> >> Once again, this time really adding stable.

> >>

> >> On 2020-06-03 04:31, Martin K. Petersen wrote:

> >>> On Thu, 28 May 2020 21:31:08 +0200, Bodo Stroesser wrote:

> >>>

> >>>> 1) If remaining ring space before the end of the ring is

> >>>>       smaller then the next cmd to write, tcmu writes a padding

> >>>>       entry which fills the remaining space at the end of the

> >>>>       ring.

> >>>>       Then tcmu calls tcmu_flush_dcache_range() with the size

> >>>>       of struct tcmu_cmd_entry as data length to flush.

> >>>>       If the space filled by the padding was smaller then

> >>>>       tcmu_cmd_entry, tcmu_flush_dcache_range() is called for

> >>>>       an address range reaching behind the end of the vmalloc'ed

> >>>>       ring.

> >>>>       tcmu_flush_dcache_range() in a loop calls

> >>>>          flush_dcache_page(virt_to_page(start));

> >>>>       for every page being part of the range. On x86 the line is

> >>>>       optimized out by the compiler, as flush_dcache_page() is

> >>>>       empty on x86.

> >>>>       But I assume the above can cause trouble on other

> >>>>       architectures that really have a flush_dcache_page().

> >>>>       For paddings only the header part of an entry is relevant

> >>>>       Due to alignment rules the header always fits in the

> >>>>       remaining space, if padding is needed.

> >>>>       So tcmu_flush_dcache_range() can safely be called with

> >>>>       sizeof(entry->hdr) as the length here.

> >>>>

> >>>> [...]

> >>>

> >>> Applied to 5.8/scsi-queue, thanks!

> >>>

> >>> [1/1] scsi: target: tcmu: Fix size in calls to tcmu_flush_dcache_range

> >>>          https://git.kernel.org/mkp/scsi/c/8c4e0f212398

> >>>

> >>

> >> The full commit of this patch is:

> >>       8c4e0f212398cdd1eb4310a5981d06a723cdd24f

> >>

> >> This patch is the first of four patches that are necessary to run tcmu

> >> on ARM without crash. For details please see

> >>       https://bugzilla.kernel.org/show_bug.cgi?id=208045

> >> Upsteam commits of patches 2,3, and 4 are:

> >>     2: 3c58f737231e "scsi: target: tcmu: Optimize use of flush_dcache_page"

> >>     3: 3145550a7f8b "scsi: target: tcmu: Fix crash in tcmu_flush_dcache_range

> >> on ARM"

> >>     4: 5a0c256d96f0 "scsi: target: tcmu: Fix crash on ARM during cmd

> >> completion"

> >>

> >> Since patches 3 and 4 already were accepted for 5.8, 5.4, and 4.19, and

> >> I sent a request to add patch 2 about 1 hour ago, please consider adding

> >> this patch to 5.4 and 4.19, because without it tcmu on ARM will still

> >> crash.

> > 

> > I don't see such a request, and am confused now.

> > 

> > What exact commits do you want backported, and to what trees?

> > 

> > thanks,

> > 

> > greg k-h

> > 

> 

> Sorry for the confusion.

> 

> The subject of the request I mentioned is

>     "Re: [PATCH v2 0/2] scsi: target: tcmu: fix crashes on ARM"

> because it is for the first patch of a small series of two.

> 

> Please backport to kernels 4.19 and 5.4 (it is part of 5.8 from beginning):

>   8c4e0f212398 "scsi: target: tcmu: fix size in calls to tcmu_flush_dcache_range"

> 

> Please backport to kernels 4.19, 5.4 and 5.8:

>   3c58f737231e "scsi: target: tcmu: Optimize use of flush_dcache_page"

> 

> Backporting to 4.14 or earlier AFAICS would need more work, especially testing.

> I don't think that its worth it.


Thanks, both now queued up.

greg k-h
diff mbox series

Patch

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index f769bb1e3735..cdb4848d23c6 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1026,7 +1026,7 @@  static int queue_cmd_ring(struct tcmu_cmd *tcmu_cmd, sense_reason_t *scsi_err)
 		entry->hdr.cmd_id = 0; /* not used for PAD */
 		entry->hdr.kflags = 0;
 		entry->hdr.uflags = 0;
-		tcmu_flush_dcache_range(entry, sizeof(*entry));
+		tcmu_flush_dcache_range(entry, sizeof(entry->hdr));
 
 		UPDATE_HEAD(mb->cmd_head, pad_size, udev->cmdr_size);
 		tcmu_flush_dcache_range(mb, sizeof(*mb));
@@ -1084,7 +1084,7 @@  static int queue_cmd_ring(struct tcmu_cmd *tcmu_cmd, sense_reason_t *scsi_err)
 	cdb_off = CMDR_OFF + cmd_head + base_command_size;
 	memcpy((void *) mb + cdb_off, se_cmd->t_task_cdb, scsi_command_size(se_cmd->t_task_cdb));
 	entry->req.cdb_off = cdb_off;
-	tcmu_flush_dcache_range(entry, sizeof(*entry));
+	tcmu_flush_dcache_range(entry, command_size);
 
 	UPDATE_HEAD(mb->cmd_head, command_size, udev->cmdr_size);
 	tcmu_flush_dcache_range(mb, sizeof(*mb));