[v2,2/3] scsi: target: tcmu: Fix possible data corruption

When tcmu_vma_fault() gets one page successfully, before the current
context completes page fault procedure, find_free_blocks() may run in
and call unmap_mapping_range() to unmap this page. Assume when
find_free_blocks() completes its job firstly, previous page fault
procedure starts to run again and completes, then one truncated page has
beed mapped to use space, but note that tcmu_vma_fault() has gotten one
refcount for this page, so any other subsystem won't use this page,
unless later the use space addr is unmapped.

If another command runs in later and needs to extends dbi_thresh, it may
reuse the corresponding slot to previous page in data_bitmap, then thouth
we'll allocate new page for this slot in data_area, but no page fault will
happen again, because we have a valid map, real request's data will lose.

To fix this issue, when extending dbi_thresh, we'll need to call
unmap_mapping_range() to unmap use space data area which may exist,
which I think it's a simple method.

Filesystem implementations will also run into this issue, but they
ususally lock page when vm_operations_struct->fault gets one page, and
unlock page after finish_fault() completes. In truncate sides, they
lock pages in truncate_inode_pages() to protect race with page fault.
We can also have similar codes like filesystem to fix this issue.

Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
---
 drivers/target/target_core_user.c | 5 +++++
 1 file changed, 5 insertions(+)

On 23.03.22 14:49, Xiaoguang Wang wrote:
> When tcmu_vma_fault() gets one page successfully, before the current
> context completes page fault procedure, find_free_blocks() may run in
> and call unmap_mapping_range() to unmap this page. Assume when
> find_free_blocks() completes its job firstly, previous page fault
> procedure starts to run again and completes, then one truncated page has
> beed mapped to use space, but note that tcmu_vma_fault() has gotten one
> refcount for this page, so any other subsystem won't use this page,
> unless later the use space addr is unmapped.
> 
> If another command runs in later and needs to extends dbi_thresh, it may
> reuse the corresponding slot to previous page in data_bitmap, then thouth
> we'll allocate new page for this slot in data_area, but no page fault will
> happen again, because we have a valid map, real request's data will lose.

I don't think, this is a safe fix. It is possible that not only
find_free_blocks runs before page fault procedure completes, but also
allocation for next cmd happens. In that case the new call to
unmap_mapping_range would also happen before page fault completes ->
data corruption.

AFAIK, no one ever has seen this this bug in real life, as
find_free_blocks only runs seldomly and userspace would have to access
a data page the very first time while the cmd that owned this page
already has been completed by userspace. Therefore I think we should
apply a perfect fix only.

I'm wondering whether there really is such a race. If so, couldn't the
same race happen in other drivers or even when truncating mapped files?


> 
> To fix this issue, when extending dbi_thresh, we'll need to call
> unmap_mapping_range() to unmap use space data area which may exist,
> which I think it's a simple method.
> 
> Filesystem implementations will also run into this issue, but they
> ususally lock page when vm_operations_struct->fault gets one page, and
> unlock page after finish_fault() completes. In truncate sides, they
> lock pages in truncate_inode_pages() to protect race with page fault.
> We can also have similar codes like filesystem to fix this issue.
> 
> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
> ---
>   drivers/target/target_core_user.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 06a5c4086551..9196188504ec 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -862,6 +862,7 @@ static int tcmu_alloc_data_space(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
>   	if (space < cmd->dbi_cnt) {
>   		unsigned long blocks_left =
>   				(udev->max_blocks - udev->dbi_thresh) + space;
> +		loff_t off, len;
>   
>   		if (blocks_left < cmd->dbi_cnt) {
>   			pr_debug("no data space: only %lu available, but ask for %u\n",
> @@ -870,6 +871,10 @@ static int tcmu_alloc_data_space(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
>   			return -1;
>   		}
>   
> +		off = udev->data_off + (loff_t)udev->dbi_thresh * udev->data_blk_size;
> +		len = cmd->dbi_cnt * udev->data_blk_size;
> +		unmap_mapping_range(udev->inode->i_mapping, off, len, 1);
> +
>   		udev->dbi_thresh += cmd->dbi_cnt;
>   		if (udev->dbi_thresh > udev->max_blocks)
>   			udev->dbi_thresh = udev->max_blocks;

hi,

> On 23.03.22 14:49, Xiaoguang Wang wrote:
>> When tcmu_vma_fault() gets one page successfully, before the current
>> context completes page fault procedure, find_free_blocks() may run in
>> and call unmap_mapping_range() to unmap this page. Assume when
>> find_free_blocks() completes its job firstly, previous page fault
>> procedure starts to run again and completes, then one truncated page has
>> beed mapped to use space, but note that tcmu_vma_fault() has gotten one
>> refcount for this page, so any other subsystem won't use this page,
>> unless later the use space addr is unmapped.
>>
>> If another command runs in later and needs to extends dbi_thresh, it may
>> reuse the corresponding slot to previous page in data_bitmap, then thouth
>> we'll allocate new page for this slot in data_area, but no page fault will
>> happen again, because we have a valid map, real request's data will lose.
>
> I don't think, this is a safe fix. It is possible that not only
> find_free_blocks runs before page fault procedure completes, but also
> allocation for next cmd happens. In that case the new call to
> unmap_mapping_range would also happen before page fault completes ->
> data corruption.
>
> AFAIK, no one ever has seen this this bug in real life, as
Yeah, I know, just find this maybe an issue by reading codes :)

> find_free_blocks only runs seldomly and userspace would have to access
> a data page the very first time while the cmd that owned this page
> already has been completed by userspace. Therefore I think we should
> apply a perfect fix only.
>
> I'm wondering whether there really is such a race. If so, couldn't the
> same race happen in other drivers or even when truncating mapped files?
Indeed, I have described how filesystem implementations avoid this issue
in patch's commit message:

  Filesystem implementations will also run into this issue, but they
  usually lock page when vm_operations_struct->fault gets one page, and
  unlock page after finish_fault() completes. In truncate sides, they
  lock pages in truncate_inode_pages() to protect race with page fault.
  We can also have similar codes like filesystem to fix this issue.


Take ext4 as example, a file in ext4 is mapped to user space, if then a truncate
operation occurs, ext4 calls truncate_pagecache():
void truncate_pagecache(struct inode *inode, loff_t newsize)
{
        struct address_space *mapping = inode->i_mapping;
        loff_t holebegin = round_up(newsize, PAGE_SIZE);

        /*
         * unmap_mapping_range is called twice, first simply for
         * efficiency so that truncate_inode_pages does fewer
         * single-page unmaps.  However after this first call, and
         * before truncate_inode_pages finishes, it is possible for
         * private pages to be COWed, which remain after
         * truncate_inode_pages finishes, hence the second
         * unmap_mapping_range call must be made for correctness.
         */
        unmap_mapping_range(mapping, holebegin, 0, 1);
        truncate_inode_pages(mapping, newsize);
        unmap_mapping_range(mapping, holebegin, 0, 1);
}

In truncate_inode_pages(), it'll lock page and set page->mapping
to be NULL, and in ext4's filemap_fault(), it'll lock page and check whether
page->mapping has been changed, if it's true, it'll just fail the page
fault procedure.

For tcmu, though the data area's pages don't have a valid mapping,
but we can apply similar method.
In tcmu_vma_fault(), we lock the page and set VM_FAULT_LOCKED
flag, in find_free_blocks(), we firstly try to lock pages which are going
to be released, if lock_page() returns, we can ensure that there are
not inflight running page fault procedure, and following unmap_mapping_range()
will also ensure that all user maps will be cleared.
Seems that it'll resolve this possible issue, please have a check, thanks.


Regards,
Xiaoguang Wang


>
>
>>
>> To fix this issue, when extending dbi_thresh, we'll need to call
>> unmap_mapping_range() to unmap use space data area which may exist,
>> which I think it's a simple method.
>>
>> Filesystem implementations will also run into this issue, but they
>> ususally lock page when vm_operations_struct->fault gets one page, and
>> unlock page after finish_fault() completes. In truncate sides, they
>> lock pages in truncate_inode_pages() to protect race with page fault.
>> We can also have similar codes like filesystem to fix this issue.
>>
>> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
>> ---
>>   drivers/target/target_core_user.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
>> index 06a5c4086551..9196188504ec 100644
>> --- a/drivers/target/target_core_user.c
>> +++ b/drivers/target/target_core_user.c
>> @@ -862,6 +862,7 @@ static int tcmu_alloc_data_space(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
>>       if (space < cmd->dbi_cnt) {
>>           unsigned long blocks_left =
>>                   (udev->max_blocks - udev->dbi_thresh) + space;
>> +        loff_t off, len;
>>             if (blocks_left < cmd->dbi_cnt) {
>>               pr_debug("no data space: only %lu available, but ask for %u\n",
>> @@ -870,6 +871,10 @@ static int tcmu_alloc_data_space(struct tcmu_dev *udev, struct tcmu_cmd *cmd,
>>               return -1;
>>           }
>>   +        off = udev->data_off + (loff_t)udev->dbi_thresh * udev->data_blk_size;
>> +        len = cmd->dbi_cnt * udev->data_blk_size;
>> +        unmap_mapping_range(udev->inode->i_mapping, off, len, 1);
>> +
>>           udev->dbi_thresh += cmd->dbi_cnt;
>>           if (udev->dbi_thresh > udev->max_blocks)
>>               udev->dbi_thresh = udev->max_blocks;

On 05.04.22 18:03, Xiaoguang Wang wrote:
> hi,
> 
>> On 23.03.22 14:49, Xiaoguang Wang wrote:
>>> When tcmu_vma_fault() gets one page successfully, before the current
>>> context completes page fault procedure, find_free_blocks() may run in
>>> and call unmap_mapping_range() to unmap this page. Assume when
>>> find_free_blocks() completes its job firstly, previous page fault
>>> procedure starts to run again and completes, then one truncated page has
>>> beed mapped to use space, but note that tcmu_vma_fault() has gotten one
>>> refcount for this page, so any other subsystem won't use this page,
>>> unless later the use space addr is unmapped.
>>>
>>> If another command runs in later and needs to extends dbi_thresh, it may
>>> reuse the corresponding slot to previous page in data_bitmap, then thouth
>>> we'll allocate new page for this slot in data_area, but no page fault will
>>> happen again, because we have a valid map, real request's data will lose.
>>
>> I don't think, this is a safe fix. It is possible that not only
>> find_free_blocks runs before page fault procedure completes, but also
>> allocation for next cmd happens. In that case the new call to
>> unmap_mapping_range would also happen before page fault completes ->
>> data corruption.
>>
>> AFAIK, no one ever has seen this this bug in real life, as
> Yeah, I know, just find this maybe an issue by reading codes :)
> 
>> find_free_blocks only runs seldomly and userspace would have to access
>> a data page the very first time while the cmd that owned this page
>> already has been completed by userspace. Therefore I think we should
>> apply a perfect fix only.
>>
>> I'm wondering whether there really is such a race. If so, couldn't the
>> same race happen in other drivers or even when truncating mapped files?
> Indeed, I have described how filesystem implementations avoid this issue
> in patch's commit message:
> 
>    Filesystem implementations will also run into this issue, but they
>    usually lock page when vm_operations_struct->fault gets one page, and
>    unlock page after finish_fault() completes. In truncate sides, they
>    lock pages in truncate_inode_pages() to protect race with page fault.
>    We can also have similar codes like filesystem to fix this issue.
> 
> 
> Take ext4 as example, a file in ext4 is mapped to user space, if then a truncate
> operation occurs, ext4 calls truncate_pagecache():
> void truncate_pagecache(struct inode *inode, loff_t newsize)
> {
>          struct address_space *mapping = inode->i_mapping;
>          loff_t holebegin = round_up(newsize, PAGE_SIZE);
> 
>          /*
>           * unmap_mapping_range is called twice, first simply for
>           * efficiency so that truncate_inode_pages does fewer
>           * single-page unmaps.  However after this first call, and
>           * before truncate_inode_pages finishes, it is possible for
>           * private pages to be COWed, which remain after
>           * truncate_inode_pages finishes, hence the second
>           * unmap_mapping_range call must be made for correctness.
>           */
>          unmap_mapping_range(mapping, holebegin, 0, 1);
>          truncate_inode_pages(mapping, newsize);
>          unmap_mapping_range(mapping, holebegin, 0, 1);
> }
> 
> In truncate_inode_pages(), it'll lock page and set page->mapping
> to be NULL, and in ext4's filemap_fault(), it'll lock page and check whether
> page->mapping has been changed, if it's true, it'll just fail the page
> fault procedure.
> 
> For tcmu, though the data area's pages don't have a valid mapping,
> but we can apply similar method.
> In tcmu_vma_fault(), we lock the page and set VM_FAULT_LOCKED
> flag,

Yeah, looking into page fault handling I'm wondering why tcmu didn't do
that from the beginning!

> in find_free_blocks(), we firstly try to lock pages which are going
> to be released, if lock_page() returns,

I assume, we immediately unlock the page again, right?

> we can ensure that there are
> not inflight running page fault procedure, and following unmap_mapping_range()
> will also ensure that all user maps will be cleared.
> Seems that it'll resolve this possible issue, please have a check, thanks.

AFAICS, this is the clean solution we were searching for.

Thank you
Bodo