From: Hao Lan <lanhao@huawei.com>

This patch modifies the implementation of debugfs:
When the user process stops unexpectedly, not all data of the file system
is read. In this case, the save_buf pointer is not released. When the user
process is called next time, save_buf is used to copy the cached data
to the user space. As a result, the queried data is inconsistent. To solve
this problem, determine whether the function is invoked for the first time
based on the value of *ppos. If *ppos is 0, obtain the actual data.

Fixes: 5e69ea7ee2a6 ("net: hns3: refactor the debugfs process")
Signed-off-by: Hao Lan <lanhao@huawei.com>
Signed-off-by: Guangwei Zhang <zhangwangwei6@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
index 807eb3bbb11c..841e5af7b2be 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
@@ -1293,8 +1293,10 @@ static ssize_t hns3_dbg_read(struct file *filp, char __user *buffer,
 
 		/* save the buffer addr until the last read operation */
 		*save_buf = read_buf;
+	}
 
-		/* get data ready for the first time to read */
+	/* get data ready for the first time to read */
+	if (!*ppos) {
 		ret = hns3_dbg_read_cmd(dbg_data, hns3_dbg_cmd[index].cmd,
 					read_buf, hns3_dbg_cmd[index].buf_len);
 		if (ret)
-- 
2.33.0

On Thu, 7 Nov 2024 21:30:19 +0800 Jijie Shao wrote:
> Subject: [PATCH RESEND net 3/7] net: hns3: Resolved the issue that the debugfs query result is inconsistent.
> Date: Thu, 7 Nov 2024 21:30:19 +0800
> X-Mailer: git-send-email 2.30.0
> 
> From: Hao Lan <lanhao@huawei.com>
> 
> This patch modifies the implementation of debugfs:
> When the user process stops unexpectedly, not all data of the file system
> is read. In this case, the save_buf pointer is not released. When the user
> process is called next time, save_buf is used to copy the cached data
> to the user space. As a result, the queried data is inconsistent. To solve
> this problem, determine whether the function is invoked for the first time
> based on the value of *ppos. If *ppos is 0, obtain the actual data.

What do you mean by "inconsistent" ?
What if two processes read the file at once?

on 2024/11/12 9:25, Jakub Kicinski wrote:
> On Thu, 7 Nov 2024 21:30:19 +0800 Jijie Shao wrote:
>> Subject: [PATCH RESEND net 3/7] net: hns3: Resolved the issue that the debugfs query result is inconsistent.
>> Date: Thu, 7 Nov 2024 21:30:19 +0800
>> X-Mailer: git-send-email 2.30.0
>>
>> From: Hao Lan <lanhao@huawei.com>
>>
>> This patch modifies the implementation of debugfs:
>> When the user process stops unexpectedly, not all data of the file system
>> is read. In this case, the save_buf pointer is not released. When the user
>> process is called next time, save_buf is used to copy the cached data
>> to the user space. As a result, the queried data is inconsistent. To solve
>> this problem, determine whether the function is invoked for the first time
>> based on the value of *ppos. If *ppos is 0, obtain the actual data.
> What do you mean by "inconsistent" ?
> What if two processes read the file at once?

inconsistent：
Before this modification,
if the previous read operation is stopped before complete, the buffer is not released.
In the next read operation (perhaps after a long time), the driver does not read again.
Instead, the driver returns the bufffer content, which causes outdated data to be obtained.
As a result, the obtained data is inconsistent with the actual data.

In this patch, ppos is used to determine whether a new read operation is performed.
If yes, the driver updates the data in the buffer to ensure that the queried data is fresh.
But, if two processes read the same file at once, The read operation that ends first releases the buffer.
As a result, the other read operation re-alloc buffer memory. However, because the value of ppos is not 0,
the data is not updated again. As a result, the queried data is truncated.

This is a bug and I will fix it in the next version.

Thanks
Jijie Shao

On Wed, 13 Nov 2024 13:59:32 +0800 Jijie Shao wrote:
> inconsistent：
> Before this modification,
> if the previous read operation is stopped before complete, the buffer is not released.
> In the next read operation (perhaps after a long time), the driver does not read again.
> Instead, the driver returns the bufffer content, which causes outdated data to be obtained.
> As a result, the obtained data is inconsistent with the actual data.

I think the word "stale" would fit the situation better.

> In this patch, ppos is used to determine whether a new read operation is performed.
> If yes, the driver updates the data in the buffer to ensure that the queried data is fresh.
> But, if two processes read the same file at once, The read operation that ends first releases the buffer.
> As a result, the other read operation re-alloc buffer memory. However, because the value of ppos is not 0,
> the data is not updated again. As a result, the queried data is truncated.
> 
> This is a bug and I will fix it in the next version.

Let's say two reads are necessary to read the data:

 reader A                  reader B
  read()
   - alloc
   - hns3_dbg_read_cmd()
                           read()
                           read()
                           read(EOF) 
                            - free
  read()
   - alloc
   - hns3_dbg_read_cmd()
  read(EOF) 
   - free

The data for read A is half from one hns3_dbg_read_cmd() and half from
another. Does it not cause any actual inconsistency?

Also, just to be sure, it's not possible to lseek on these files, right?