linux-kernel - Re: [PATCH RESEND net 3/7] net: hns3: Resolved the issue that the debugfs query result is inconsistent.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1448b4a1-235c-4abe-9f95-fbf6e7f9d640@huawei.com>
Date: Fri, 13 Dec 2024 21:11:49 +0800
From: Jijie Shao <shaojijie@...wei.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <shaojijie@...wei.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<pabeni@...hat.com>, <andrew+netdev@...n.ch>, <horms@...nel.org>,
	<shenjian15@...wei.com>, <salil.mehta@...wei.com>, <liuyonglong@...wei.com>,
	<wangpeiyang1@...wei.com>, <chenhao418@...wei.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND net 3/7] net: hns3: Resolved the issue that the
 debugfs query result is inconsistent.


on 2024/12/10 5:13, Jakub Kicinski wrote:
> On Mon, 9 Dec 2024 22:14:37 +0800 Jijie Shao wrote:
>> Another way is seq_file, which may be a solution,
>> as far as I know, each seq_file has a separate buffer and can be expanded automatically.
>> So it might be possible to solve the problem
>> But even if the solution is feasible, this will require a major refactoring of hns3 debugfs
> seq_file is generally used for text output
>
> can you not hook in the allocation and execution of the cmd into the
> .open handler and freeing in to the .close handler? You already use
> explicit file_ops for this file.


Thank you very much for your advice.

When I modified the code according to your comments,
I found that the problem mentioned in this path can be solved.
I implement .open() and .release() handler for debugfs file_operations，
Move allocation buffer and execution of the cmd to the .open() handler
and freeing in to the .release() handler.
Also allocate separate buffer for each reader and associate the buffer with the file pointer.
In this case, there is no shared buffer, which causes data inconsistency.

However, a new problem is introduced：
If the framework does not call .release() for some reason, the buffer cannot be freed, causing memory leakage.
Maybe it's acceptable？


  static ssize_t hns3_dbg_read(struct file *filp, char __user *buffer,
  			     size_t count, loff_t *ppos)
  {
-	struct hns3_dbg_data *dbg_data = filp->private_data;
+	char *buf = filp->private_data;
+
+	return simple_read_from_buffer(buffer, count, ppos, buf, strlen(buf));
+}
+
+static int hns3_dbg_open(struct inode *inode, struct file *filp)
+{
+	struct hns3_dbg_data *dbg_data = inode->i_private;
  	struct hnae3_handle *handle = dbg_data->handle;
  	struct hns3_nic_priv *priv = handle->priv;
-	ssize_t size = 0;
-	char **save_buf;
-	char *read_buf;
  	u32 index;
+	char *buf;
  	int ret;
  
+	if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
+	    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
+		return -EBUSY;
+
  	ret = hns3_dbg_get_cmd_index(dbg_data, &index);
  	if (ret)
  		return ret;
  
-	mutex_lock(&handle->dbgfs_lock);
-	save_buf = &handle->dbgfs_buf[index];
-
-	if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
-	    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state)) {
-		ret = -EBUSY;
-		goto out;
-	}
-
-	if (*save_buf) {
-		read_buf = *save_buf;
-	} else {
-		read_buf = kvzalloc(hns3_dbg_cmd[index].buf_len, GFP_KERNEL);
-		if (!read_buf) {
-			ret = -ENOMEM;
-			goto out;
-		}
-
-		/* save the buffer addr until the last read operation */
-		*save_buf = read_buf;
-
-		/* get data ready for the first time to read */
-		ret = hns3_dbg_read_cmd(dbg_data, hns3_dbg_cmd[index].cmd,
-					read_buf, hns3_dbg_cmd[index].buf_len);
-		if (ret)
-			goto out;
-	}
+	buf = kvzalloc(hns3_dbg_cmd[index].buf_len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
  
-	size = simple_read_from_buffer(buffer, count, ppos, read_buf,
-				       strlen(read_buf));
-	if (size > 0) {
-		mutex_unlock(&handle->dbgfs_lock);
-		return size;
+	ret = hns3_dbg_read_cmd(dbg_data, hns3_dbg_cmd[index].cmd,
+				buf, hns3_dbg_cmd[index].buf_len);
+	if (ret) {
+		kvfree(buf);
+		return ret;
  	}
  
-out:
-	/* free the buffer for the last read operation */
-	if (*save_buf) {
-		kvfree(*save_buf);
-		*save_buf = NULL;
-	}
+	filp->private_data = buf;
+	return 0;
+}
  
-	mutex_unlock(&handle->dbgfs_lock);
-	return ret;
+static int hns3_dbg_release(struct inode *inode, struct file *filp)
+{
+	kvfree(filp->private_data);
+	filp->private_data = NULL;
+	return 0;
  }
  
  static const struct file_operations hns3_dbg_fops = {
  	.owner = THIS_MODULE,
-	.open  = simple_open,
+	.open  = hns3_dbg_open,
  	.read  = hns3_dbg_read,
+	.release = hns3_dbg_release,
  };