diff mbox series

[1/2] libceph: fail the sparse-read if there still has data in socket

Message ID 20231208043305.91249-2-xiubli@redhat.com (mailing list archive)
State New, archived
Headers show
Series libceph: fix sparse-read failure bug | expand

Commit Message

Xiubo Li Dec. 8, 2023, 4:33 a.m. UTC 
From: Xiubo Li <xiubli@redhat.com>

Once this happens that means there have bugs.

URL: https://tracker.ceph.com/issues/63586
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 net/ceph/osd_client.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jeffrey Layton Dec. 8, 2023, 11:23 a.m. UTC | #1 
On Fri, 2023-12-08 at 12:33 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> Once this happens that means there have bugs.
> 
> URL: https://tracker.ceph.com/issues/63586
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  net/ceph/osd_client.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 5753036d1957..848ef19055a0 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -5912,10 +5912,12 @@ static int osd_sparse_read(struct ceph_connection *con,
>  		fallthrough;
>  	case CEPH_SPARSE_READ_DATA:
>  		if (sr->sr_index >= count) {
> -			if (sr->sr_datalen && count)
> +			if (sr->sr_datalen) {
>  				pr_warn_ratelimited("sr_datalen %u sr_index %d count %u\n",
>  						    sr->sr_datalen, sr->sr_index,
>  						    count);
> +				return -EREMOTEIO;
> +			}
>  
>  			sr->sr_state = CEPH_SPARSE_READ_HDR;
>  			goto next_op;

Nice catch. You can add:

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Ilya Dryomov Dec. 8, 2023, 11:31 a.m. UTC | #2 
On Fri, Dec 8, 2023 at 5:34 AM <xiubli@redhat.com> wrote:
>
> From: Xiubo Li <xiubli@redhat.com>
>
> Once this happens that means there have bugs.
>
> URL: https://tracker.ceph.com/issues/63586
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  net/ceph/osd_client.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 5753036d1957..848ef19055a0 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -5912,10 +5912,12 @@ static int osd_sparse_read(struct ceph_connection *con,
>                 fallthrough;
>         case CEPH_SPARSE_READ_DATA:
>                 if (sr->sr_index >= count) {
> -                       if (sr->sr_datalen && count)
> +                       if (sr->sr_datalen) {
>                                 pr_warn_ratelimited("sr_datalen %u sr_index %d count %u\n",
>                                                     sr->sr_datalen, sr->sr_index,
>                                                     count);
> +                               return -EREMOTEIO;
> +                       }
>
>                         sr->sr_state = CEPH_SPARSE_READ_HDR;
>                         goto next_op;
> --
> 2.43.0
>

Hi Xiubo,

There is a patch in linux-next, also from you, which is conflicting
with this one: cca19d307d35 ("libceph: check the data length when
sparse read finishes").  Do you want it replaced?

Thanks,

                Ilya
Xiubo Li Dec. 8, 2023, 2:28 p.m. UTC | #3 
On 12/8/23 19:31, Ilya Dryomov wrote:
> On Fri, Dec 8, 2023 at 5:34 AM <xiubli@redhat.com> wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> Once this happens that means there have bugs.
>>
>> URL: https://tracker.ceph.com/issues/63586
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   net/ceph/osd_client.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
>> index 5753036d1957..848ef19055a0 100644
>> --- a/net/ceph/osd_client.c
>> +++ b/net/ceph/osd_client.c
>> @@ -5912,10 +5912,12 @@ static int osd_sparse_read(struct ceph_connection *con,
>>                  fallthrough;
>>          case CEPH_SPARSE_READ_DATA:
>>                  if (sr->sr_index >= count) {
>> -                       if (sr->sr_datalen && count)
>> +                       if (sr->sr_datalen) {
>>                                  pr_warn_ratelimited("sr_datalen %u sr_index %d count %u\n",
>>                                                      sr->sr_datalen, sr->sr_index,
>>                                                      count);
>> +                               return -EREMOTEIO;
>> +                       }
>>
>>                          sr->sr_state = CEPH_SPARSE_READ_HDR;
>>                          goto next_op;
>> --
>> 2.43.0
>>
> Hi Xiubo,
>
> There is a patch in linux-next, also from you, which is conflicting
> with this one: cca19d307d35 ("libceph: check the data length when
> sparse read finishes").  Do you want it replaced?

Yeah, let me fold them.

Thanks Jeff and Ilya.


> Thanks,
>
>                  Ilya
>
Ilya Dryomov Dec. 8, 2023, 3:28 p.m. UTC | #4 
On Fri, Dec 8, 2023 at 4:18 PM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 12/8/23 19:31, Ilya Dryomov wrote:
>
> On Fri, Dec 8, 2023 at 5:34 AM <xiubli@redhat.com> wrote:
>
> From: Xiubo Li <xiubli@redhat.com>
>
> Once this happens that means there have bugs.
>
> URL: https://tracker.ceph.com/issues/63586
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  net/ceph/osd_client.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 5753036d1957..848ef19055a0 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -5912,10 +5912,12 @@ static int osd_sparse_read(struct ceph_connection *con,
>                 fallthrough;
>         case CEPH_SPARSE_READ_DATA:
>                 if (sr->sr_index >= count) {
> -                       if (sr->sr_datalen && count)
> +                       if (sr->sr_datalen) {
>                                 pr_warn_ratelimited("sr_datalen %u sr_index %d count %u\n",
>                                                     sr->sr_datalen, sr->sr_index,
>                                                     count);
> +                               return -EREMOTEIO;
> +                       }
>
>                         sr->sr_state = CEPH_SPARSE_READ_HDR;
>                         goto next_op;
> --
> 2.43.0
>
> Hi Xiubo,
>
> There is a patch in linux-next, also from you, which is conflicting
> with this one: cca19d307d35 ("libceph: check the data length when
> sparse read finishes").  Do you want it replaced?
>
> Ilya,
>
> I found the commit cca19d307d35 has already in the master branch. Could you fold and update it ?

I would like to see the entire fix first.  You seem to be going back
and forth between just issuing a warning or also returning an error and
the precise if condition there, so I'm starting to think that the bug
is not fully understood and neither patch might be necessary.

Thanks,

                Ilya
Xiubo Li Dec. 8, 2023, 4:07 p.m. UTC | #5 
On 12/8/23 23:28, Ilya Dryomov wrote:
> On Fri, Dec 8, 2023 at 4:18 PM Xiubo Li <xiubli@redhat.com> wrote:
>>
>> On 12/8/23 19:31, Ilya Dryomov wrote:
>>
>> On Fri, Dec 8, 2023 at 5:34 AM <xiubli@redhat.com> wrote:
>>
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> Once this happens that means there have bugs.
>>
>> URL: https://tracker.ceph.com/issues/63586
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   net/ceph/osd_client.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
>> index 5753036d1957..848ef19055a0 100644
>> --- a/net/ceph/osd_client.c
>> +++ b/net/ceph/osd_client.c
>> @@ -5912,10 +5912,12 @@ static int osd_sparse_read(struct ceph_connection *con,
>>                  fallthrough;
>>          case CEPH_SPARSE_READ_DATA:
>>                  if (sr->sr_index >= count) {
>> -                       if (sr->sr_datalen && count)
>> +                       if (sr->sr_datalen) {
>>                                  pr_warn_ratelimited("sr_datalen %u sr_index %d count %u\n",
>>                                                      sr->sr_datalen, sr->sr_index,
>>                                                      count);
>> +                               return -EREMOTEIO;
>> +                       }
>>
>>                          sr->sr_state = CEPH_SPARSE_READ_HDR;
>>                          goto next_op;
>> --
>> 2.43.0
>>
>> Hi Xiubo,
>>
>> There is a patch in linux-next, also from you, which is conflicting
>> with this one: cca19d307d35 ("libceph: check the data length when
>> sparse read finishes").  Do you want it replaced?
>>
>> Ilya,
>>
>> I found the commit cca19d307d35 has already in the master branch. Could you fold and update it ?
> I would like to see the entire fix first.  You seem to be going back
> and forth between just issuing a warning or also returning an error and
> the precise if condition there, so I'm starting to think that the bug
> is not fully understood and neither patch might be necessary.

Sure, I just sent out the second version.

Thanks

- Xiubo


> Thanks,
>
>                  Ilya
>
diff mbox series

Patch

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 5753036d1957..848ef19055a0 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -5912,10 +5912,12 @@  static int osd_sparse_read(struct ceph_connection *con,
 		fallthrough;
 	case CEPH_SPARSE_READ_DATA:
 		if (sr->sr_index >= count) {
-			if (sr->sr_datalen && count)
+			if (sr->sr_datalen) {
 				pr_warn_ratelimited("sr_datalen %u sr_index %d count %u\n",
 						    sr->sr_datalen, sr->sr_index,
 						    count);
+				return -EREMOTEIO;
+			}
 
 			sr->sr_state = CEPH_SPARSE_READ_HDR;
 			goto next_op;