Skip to contentSkip to conversation

crypto: hisilicon/qm - allow smaller reads in debugfs

PATCH Superseded Dan Carpenter 3 participants 3 messages May 28, 2020 – Jun 1, 2020

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 crypto: hisilicon/qm - allow smaller reads in debugfs

Originally this code rejected any read less than 256 bytes. There is no need for this artificial limit.

Also I have changed the snprintf() functions to scnprintf(). The difference is that snprintf() returns the number of bytes which would have been copied if there were enough space and scnprintf() returns the number of bytes which were actually copied. It doesn't matter here because the strings are very short so they can't go over 256 bytes.

---
 drivers/crypto/hisilicon/qm.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)
drivers/crypto/hisilicon/qm.c +8 -10
index a781c02251980..9c0c9f500d91d 100644
@@ -1076,16 +1076,15 @@ static ssize_t qm_cmd_read(struct file *filp, char __user *buffer,
 	if (*pos)
 		return 0;
 
-	if (count < QM_DBG_READ_LEN)
-		return -ENOSPC;
-
-	len = snprintf(buf, QM_DBG_READ_LEN, "%s\n",
+	len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n",
 		       "Please echo help to cmd to get help information");
 
+	len = min_t(size_t, len, count);
 	if (copy_to_user(buffer, buf, len))
 		return -EFAULT;
 
-	return (*pos = len);
+	*pos = len;
+	return len;
 }
 
 static void *qm_ctx_alloc(struct hisi_qm *qm, size_t ctx_size,
@@ -2710,19 +2709,18 @@ static ssize_t qm_status_read(struct file *filp, char __user *buffer,
 	if (*pos)
 		return 0;
 
-	if (count < QM_DBG_READ_LEN)
-		return -ENOSPC;
-
 	val = atomic_read(&qm->status.flags);
-	len = snprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
+	len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
 	if (!len)
 		return -EFAULT;
 
+	len = min_t(size_t, len, count);
 	cp_len = copy_to_user(buffer, buf, len);
 	if (cp_len)
 		return -EFAULT;
 
-	return (*pos = len);
+	*pos = len;
+	return len;
 }
 
 static const struct file_operations qm_status_fops = {
Signature · 1 line
2.26.2
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 10 lines · Dan Carpenter

Originally this code rejected any read less than 256 bytes. There is no need for this artificial limit.

Also I have changed the snprintf() functions to scnprintf(). The difference is that snprintf() returns the number of bytes which would have been copied if there were enough space and scnprintf() returns the number of bytes which were actually copied. It doesn't matter here because the strings are very short so they can't go over 256 bytes.

Looks good to me, thanks!

Quoted 54 lines
---
 drivers/crypto/hisilicon/qm.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)
drivers/crypto/hisilicon/qm.c +8 -10
index a781c02251980..9c0c9f500d91d 100644
@@ -1076,16 +1076,15 @@ static ssize_t qm_cmd_read(struct file *filp, char __user *buffer,
 	if (*pos)
 		return 0;
 
-	if (count < QM_DBG_READ_LEN)
-		return -ENOSPC;
-
-	len = snprintf(buf, QM_DBG_READ_LEN, "%s\n",
+	len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n",
 		       "Please echo help to cmd to get help information");
 
+	len = min_t(size_t, len, count);
 	if (copy_to_user(buffer, buf, len))
 		return -EFAULT;
 
-	return (*pos = len);
+	*pos = len;
+	return len;
 }
 
 static void *qm_ctx_alloc(struct hisi_qm *qm, size_t ctx_size,
@@ -2710,19 +2709,18 @@ static ssize_t qm_status_read(struct file *filp, char __user *buffer,
 	if (*pos)
 		return 0;
 
-	if (count < QM_DBG_READ_LEN)
-		return -ENOSPC;
-
 	val = atomic_read(&qm->status.flags);
-	len = snprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
+	len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
 	if (!len)
 		return -EFAULT;
 
+	len = min_t(size_t, len, count);
 	cp_len = copy_to_user(buffer, buf, len);
 	if (cp_len)
 		return -EFAULT;
 
-	return (*pos = len);
+	*pos = len;
+	return len;
 }
 
 static const struct file_operations qm_status_fops = {
#2 lore raw
In reply to Dan Carpenter

Hi Dan & Zhou,

Quoted 19 lines · Dan Carpenter

Originally this code rejected any read less than 256 bytes. There is no need for this artificial limit.

Also I have changed the snprintf() functions to scnprintf(). The difference is that snprintf() returns the number of bytes which would have been copied if there were enough space and scnprintf() returns the number of bytes which were actually copied. It doesn't matter here because the strings are very short so they can't go over 256 bytes.

---
 drivers/crypto/hisilicon/qm.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)
drivers/crypto/hisilicon/qm.c +0 -0
index a781c02251980..9c0c9f500d91d 100644

drivers/crypto/hisilicon/qm.c static ssize_t qm_cmd_read(struct file *filp, char __user *buffer, · line 1081

Quoted 6 lines
drivers/crypto/hisilicon/qm.c +0 -3
@@ -1076,16 +1076,15 @@ static ssize_t qm_cmd_read(struct file *filp, char __user *buffer,
 	if (*pos)
 		return 0;
 
-	if (count < QM_DBG_READ_LEN)
-		return -ENOSPC;
-

I think we should keep it, maybe you are right when using 'cat', but if you use 'read' system call this may make the user read only part of the prompt. This may break our original design. :)

Thanks,
Shukun

Quoted 39 lines
drivers/crypto/hisilicon/qm.c +8 -7
-	len = snprintf(buf, QM_DBG_READ_LEN, "%s\n",
+	len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n",
 		       "Please echo help to cmd to get help information");
 
+	len = min_t(size_t, len, count);
 	if (copy_to_user(buffer, buf, len))
 		return -EFAULT;
 
-	return (*pos = len);
+	*pos = len;
+	return len;
 }
 
 static void *qm_ctx_alloc(struct hisi_qm *qm, size_t ctx_size,
@@ -2710,19 +2709,18 @@ static ssize_t qm_status_read(struct file *filp, char __user *buffer,
 	if (*pos)
 		return 0;
 
-	if (count < QM_DBG_READ_LEN)
-		return -ENOSPC;
-
 	val = atomic_read(&qm->status.flags);
-	len = snprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
+	len = scnprintf(buf, QM_DBG_READ_LEN, "%s\n", qm_s[val]);
 	if (!len)
 		return -EFAULT;
 
+	len = min_t(size_t, len, count);
 	cp_len = copy_to_user(buffer, buf, len);
 	if (cp_len)
 		return -EFAULT;
 
-	return (*pos = len);
+	*pos = len;
+	return len;
 }
 
 static const struct file_operations qm_status_fops = {