[PATCH v1 0/2] fsi: sbefifo: fixes

[PATCH v1 0/2] fsi: sbefifo: fixes

[Ninad Palsule]

Hello,

Please review support for large write in sbefifo driver and also bug
fix.

Ninad Palsule (2):
  fsi: sbefifo: Remove write's max command length
  fsi: sbefifo: Validate pending user write

 drivers/fsi/fsi-sbefifo.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.39.2

[PATCH v1 1/2] fsi: sbefifo: Remove write's max command length

[Ninad Palsule]

This commit removes max command length check in the user write path.
This is required to support images larger than 1MB. This should not
create any issues as read path does not have this check either.

As per the original design cronus server was suppose to break up the
image into 1MB pieces but it requires restructuring of the driver.
Today driver sends EOT message on each write request so we will have to
send it only after all pieces are sent which requires large change hence
we decided to remove this check.

Testing:
  Loaded 3 MB image through cronus server.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 9912b7a6a4b9..b771dff27f7f 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -113,7 +113,6 @@ enum sbe_state
 #define SBEFIFO_TIMEOUT_IN_RSP		1000
 
 /* Other constants */
-#define SBEFIFO_MAX_USER_CMD_LEN	(0x100000 + PAGE_SIZE)
 #define SBEFIFO_RESET_MAGIC		0x52534554 /* "RSET" */
 
 struct sbefifo {
@@ -870,8 +869,6 @@ static ssize_t sbefifo_user_write(struct file *file, const char __user *buf,
 	if (!user)
 		return -EINVAL;
 	sbefifo = user->sbefifo;
-	if (len > SBEFIFO_MAX_USER_CMD_LEN)
-		return -EINVAL;
 	if (len & 3)
 		return -EINVAL;
 
-- 
2.39.2

Re: [PATCH v1 1/2] fsi: sbefifo: Remove write's max command length

[Joel Stanley]

On Thu, 7 Sept 2023 at 22:10, Ninad Palsule <ninad@linux.ibm.com> wrote:
>
> This commit removes max command length check in the user write path.
> This is required to support images larger than 1MB. This should not
> create any issues as read path does not have this check either.
>
> As per the original design cronus server was suppose to break up the
> image into 1MB pieces but it requires restructuring of the driver.

When you say "driver" you mean the kernel driver, or userspace?

This isn't a great justification for removing a bounds check.

> Today driver sends EOT message on each write request so we will have to
> send it only after all pieces are sent which requires large change hence
> we decided to remove this check.

This paragraph could be clearer. Could you try rephrasing?

Assuming we want to make this change, what is the expected maximum
transfer? Could we instead make the check be that value (3MB?).

>
> Testing:
>   Loaded 3 MB image through cronus server.
>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>  drivers/fsi/fsi-sbefifo.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 9912b7a6a4b9..b771dff27f7f 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -113,7 +113,6 @@ enum sbe_state
>  #define SBEFIFO_TIMEOUT_IN_RSP         1000
>
>  /* Other constants */
> -#define SBEFIFO_MAX_USER_CMD_LEN       (0x100000 + PAGE_SIZE)
>  #define SBEFIFO_RESET_MAGIC            0x52534554 /* "RSET" */
>
>  struct sbefifo {
> @@ -870,8 +869,6 @@ static ssize_t sbefifo_user_write(struct file *file, const char __user *buf,
>         if (!user)
>                 return -EINVAL;
>         sbefifo = user->sbefifo;
> -       if (len > SBEFIFO_MAX_USER_CMD_LEN)
> -               return -EINVAL;
>         if (len & 3)
>                 return -EINVAL;
>
> --
> 2.39.2
>

Re: [PATCH v1 1/2] fsi: sbefifo: Remove write's max command length

[Ninad Palsule]

Hi Joel,

On 9/11/23 01:03, Joel Stanley wrote:
> On Thu, 7 Sept 2023 at 22:10, Ninad Palsule <ninad@linux.ibm.com> wrote:
>> This commit removes max command length check in the user write path.
>> This is required to support images larger than 1MB. This should not
>> create any issues as read path does not have this check either.
>>
>> As per the original design cronus server was suppose to break up the
>> image into 1MB pieces but it requires restructuring of the driver.
> When you say "driver" you mean the kernel driver, or userspace?
>
> This isn't a great justification for removing a bounds check.
I have improved the comment. Added length check back.
>
>> Today driver sends EOT message on each write request so we will have to
>> send it only after all pieces are sent which requires large change hence
>> we decided to remove this check.
> This paragraph could be clearer. Could you try rephrasing?
>
> Assuming we want to make this change, what is the expected maximum
> transfer? Could we instead make the check be that value (3MB?).
Added length check back. I am using 4MB for some cushion.

Thanks for the review.

~Ninad

>> Testing:
>>    Loaded 3 MB image through cronus server.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   drivers/fsi/fsi-sbefifo.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
>> index 9912b7a6a4b9..b771dff27f7f 100644
>> --- a/drivers/fsi/fsi-sbefifo.c
>> +++ b/drivers/fsi/fsi-sbefifo.c
>> @@ -113,7 +113,6 @@ enum sbe_state
>>   #define SBEFIFO_TIMEOUT_IN_RSP         1000
>>
>>   /* Other constants */
>> -#define SBEFIFO_MAX_USER_CMD_LEN       (0x100000 + PAGE_SIZE)
>>   #define SBEFIFO_RESET_MAGIC            0x52534554 /* "RSET" */
>>
>>   struct sbefifo {
>> @@ -870,8 +869,6 @@ static ssize_t sbefifo_user_write(struct file *file, const char __user *buf,
>>          if (!user)
>>                  return -EINVAL;
>>          sbefifo = user->sbefifo;
>> -       if (len > SBEFIFO_MAX_USER_CMD_LEN)
>> -               return -EINVAL;
>>          if (len & 3)
>>                  return -EINVAL;
>>
>> --
>> 2.39.2
>>

[PATCH v1 2/2] fsi: sbefifo: Validate pending user write

[Ninad Palsule]

This commit fails user write operation if previous write operation is
still pending.

As per the driver design write operation only prepares the buffer, the
actual FSI write is performed on next read operation. so if buggy
application sends two back to back writes or two parallel writes then
that could cause memory leak.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index b771dff27f7f..824e2a921a25 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -874,6 +874,12 @@ static ssize_t sbefifo_user_write(struct file *file, const char __user *buf,
 
 	mutex_lock(&user->file_lock);
 
+	/* Previous write is still in progress */
+	if (user->pending_cmd) {
+		mutex_unlock(&user->file_lock);
+		return -EALREADY;
+	}
+
 	/* Can we use the pre-allocate buffer ? If not, allocate */
 	if (len <= PAGE_SIZE)
 		user->pending_cmd = user->cmd_page;
-- 
2.39.2

Re: [PATCH v1 2/2] fsi: sbefifo: Validate pending user write

[Joel Stanley]

On Thu, 7 Sept 2023 at 22:10, Ninad Palsule <ninad@linux.ibm.com> wrote:
>
> This commit fails user write operation if previous write operation is
> still pending.
>
> As per the driver design write operation only prepares the buffer, the
> actual FSI write is performed on next read operation. so if buggy
> application sends two back to back writes or two parallel writes then
> that could cause memory leak.

The driver already has this code:


>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>  drivers/fsi/fsi-sbefifo.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index b771dff27f7f..824e2a921a25 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -874,6 +874,12 @@ static ssize_t sbefifo_user_write(struct file *file, const char __user *buf,
>
>         mutex_lock(&user->file_lock);
>
> +       /* Previous write is still in progress */
> +       if (user->pending_cmd) {
> +               mutex_unlock(&user->file_lock);
> +               return -EALREADY;

That's an unusual return code. I guess it makes sense in this context.

It's good to fix the potential memory leak, and we should add code to
catch that case.

This will change the behaviour of the character device from "overwrite
the previous operation" to "reject operation until a read is
performed". Do you think there's existing code that depends on the old
behaviour?

> +       }
> +
>         /* Can we use the pre-allocate buffer ? If not, allocate */
>         if (len <= PAGE_SIZE)
>                 user->pending_cmd = user->cmd_page;
> --
> 2.39.2
>

Re: [PATCH v1 2/2] fsi: sbefifo: Validate pending user write

[Ninad Palsule]

Hi Joel,

On 9/11/23 00:52, Joel Stanley wrote:
> On Thu, 7 Sept 2023 at 22:10, Ninad Palsule <ninad@linux.ibm.com> wrote:
>> This commit fails user write operation if previous write operation is
>> still pending.
>>
>> As per the driver design write operation only prepares the buffer, the
>> actual FSI write is performed on next read operation. so if buggy
>> application sends two back to back writes or two parallel writes then
>> that could cause memory leak.
> The driver already has this code:
Yes, I have improved the comment.
>
>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   drivers/fsi/fsi-sbefifo.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
>> index b771dff27f7f..824e2a921a25 100644
>> --- a/drivers/fsi/fsi-sbefifo.c
>> +++ b/drivers/fsi/fsi-sbefifo.c
>> @@ -874,6 +874,12 @@ static ssize_t sbefifo_user_write(struct file *file, const char __user *buf,
>>
>>          mutex_lock(&user->file_lock);
>>
>> +       /* Previous write is still in progress */
>> +       if (user->pending_cmd) {
>> +               mutex_unlock(&user->file_lock);
>> +               return -EALREADY;
> That's an unusual return code. I guess it makes sense in this context.
>
> It's good to fix the potential memory leak, and we should add code to
> catch that case.
>
> This will change the behaviour of the character device from "overwrite
> the previous operation" to "reject operation until a read is
> performed". Do you think there's existing code that depends on the old
> behaviour?
I do not see any issue with this rejection. I thought user may wants to 
send reset while command is hung but that case is not valid as pending 
command will hold the lock. User can always close the connection and 
reopen if required. How do I find if this could cause the regression?
>
>> +       }
>> +
>>          /* Can we use the pre-allocate buffer ? If not, allocate */
>>          if (len <= PAGE_SIZE)
>>                  user->pending_cmd = user->cmd_page;
>> --
>> 2.39.2
>>