| Message ID | 20200528193108.9085-1-bstroesser@ts.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8c4e0f212398cdd1eb4310a5981d06a723cdd24f |
| Headers | show |
| Series | scsi: target: tcmu: fix size in calls to tcmu_flush_dcache_range | expand |
On 5/28/20 2:31 PM, 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. > > 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(-) > > 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)); > Acked-by: Mike Christie <michael.christie@oracle.com>
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
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
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
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
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
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 --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));
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(-)