Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <73ba1505-d619-466e-981a-badb2658e6cb@linux.ibm.com>
Date:   Mon, 11 Sep 2023 17:40:42 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 2/2] fsi: sbefifo: Validate pending user write
To:     Joel Stanley <joel@jms.id.au>, eajames@linux.ibm.com
Cc:     jk@ozlabs.org, alistair@popple.id.au, linux-fsi@lists.ozlabs.org,
        linux-kernel@vger.kernel.org
References: <20230907221016.2978802-1-ninad@linux.ibm.com>
 <20230907221016.2978802-3-ninad@linux.ibm.com>
 <CACPK8XcUkwNS85Ept62xyCabELHO9xTdMYvxVg2W+GZfAxTa3A@mail.gmail.com>
Content-Language: en-US
From:   Ninad Palsule <ninad@linux.ibm.com>
In-Reply-To: <CACPK8XcUkwNS85Ept62xyCabELHO9xTdMYvxVg2W+GZfAxTa3A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>>