On Mon, 2015-06-15 at 13:07 +0300, Sagi Grimberg wrote:
> On 6/12/2015 8:25 PM, Andy Grover wrote:
> > On 06/11/2015 09:58 AM, Sagi Grimberg wrote:
> >> Pointers that are mapped by kmap_atomic() + offset must
> >> be unmapped without the offset. That would cause problems
> >> if the SG element length exceeds the PAGE_SIZE limit.
> >>
> >> Signed-off-by: Sagi Grimberg <sagig@xxxxxxxxxxxx>
> >> ---
> >> drivers/target/target_core_user.c | 14 ++++++++------
> >> 1 files changed, 8 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/target/target_core_user.c
> >> b/drivers/target/target_core_user.c
> >> index 949e616..078ef6e 100644
> >> --- a/drivers/target/target_core_user.c
> >> +++ b/drivers/target/target_core_user.c
> >> @@ -260,7 +260,8 @@ static void alloc_and_scatter_data_area(struct
> >> tcmu_dev *udev,
> >>
> >> /* Uh oh, we wrapped the buffer. Must split sg across 2
> >> iovs. */
> >> if (sg->length != copy_bytes) {
> >> - from += copy_bytes;
> >> + void *from_skip = from + copy_bytes;
> >> +
> >> copy_bytes = sg->length - copy_bytes;
> >>
> >> (*iov)->iov_len = copy_bytes;
> >> @@ -270,7 +271,7 @@ static void alloc_and_scatter_data_area(struct
> >> tcmu_dev *udev,
> >> if (copy_data) {
> >> to = (void *) udev->mb_addr +
> >> udev->data_off + udev->data_head;
> >> - memcpy(to, from, copy_bytes);
> >> + memcpy(to, from_skip, copy_bytes);
> >> tcmu_flush_dcache_range(to, copy_bytes);
> >> }
> >>
> >> @@ -281,7 +282,7 @@ static void alloc_and_scatter_data_area(struct
> >> tcmu_dev *udev,
> >> copy_bytes, udev->data_size);
> >> }
> >>
> >> - kunmap_atomic(from);
> >> + kunmap_atomic(from - sg->offset);
> >
> > My impression is that we must call kunmap_atomic with the value returned
> > from kmap_atomic:
> >
> > http://lxr.free-electrons.com/source/include/linux/highmem.h#L120
>
> I agree, but $from wasn't the return value returned from kmap_atomic(),
> it was added with sg->offset. Thus we need to unmap exactly what we
> mapped. The KBUILD_BUG_ON in kunmap_atomic() prevents the call:
>
> kunmap_atomic(sg_page(sg))
>
> In the general case, it is possible that the sg->offset can increment
> the address to a different page.
>
Nice catch Sagi. Applied to target-pending/for-next.
Btw, it looks like this fix needs to be included for stable as well, but
does not apply cleanly against existing v4.1 code.
Andy, please consider fixing this up in stable code post v4.2-rc1 merge.
Thank you,
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
