| Message ID | 20231208043305.91249-3-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libceph: fix sparse-read failure bug | expand |
On Fri, Dec 8, 2023 at 5:34 AM <xiubli@redhat.com> wrote: > > From: Xiubo Li <xiubli@redhat.com> > > The messages from ceph maybe split into multiple socket packages > and we just need to wait for all the data to be availiable on the > sokcet. > > URL: https://tracker.ceph.com/issues/63586 > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > net/ceph/messenger_v1.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/net/ceph/messenger_v1.c b/net/ceph/messenger_v1.c > index f9a50d7f0d20..aff81fef932f 100644 > --- a/net/ceph/messenger_v1.c > +++ b/net/ceph/messenger_v1.c > @@ -1160,15 +1160,17 @@ static int read_partial_message(struct ceph_connection *con) > /* header */ > size = sizeof(con->v1.in_hdr); > end = size; > - ret = read_partial(con, end, size, &con->v1.in_hdr); > - if (ret <= 0) > - return ret; > + if (con->v1.in_base_pos < end) { > + ret = read_partial(con, end, size, &con->v1.in_hdr); > + if (ret <= 0) > + return ret; > > - crc = crc32c(0, &con->v1.in_hdr, offsetof(struct ceph_msg_header, crc)); > - if (cpu_to_le32(crc) != con->v1.in_hdr.crc) { > - pr_err("read_partial_message bad hdr crc %u != expected %u\n", > - crc, con->v1.in_hdr.crc); > - return -EBADMSG; > + crc = crc32c(0, &con->v1.in_hdr, offsetof(struct ceph_msg_header, crc)); > + if (cpu_to_le32(crc) != con->v1.in_hdr.crc) { > + pr_err("read_partial_message bad hdr crc %u != expected %u\n", > + crc, con->v1.in_hdr.crc); > + return -EBADMSG; > + } > } > > front_len = le32_to_cpu(con->v1.in_hdr.front_len); > -- > 2.43.0 > Hi Xiubo, This doesn't seem right to me. read_partial() is supposed to be called unconditionally. On a short read (i.e. when it's unable to fill the destination buffer -- in this case the header), it returns 0 and the stack is supposed to unroll all the way up to ceph_con_workfn(). If the destination buffer is already filled, read_partial() does nothing and returns 1. Recomputing the header crc in case read_partial_message() is called repeatedly shouldn't be an issue because con->v1.in_hdr shouldn't be modified in the interim. If it gets modified, it's a bug. It might help if you provide a step-by-step breakdown of the scenario that you are trying to address in the commit message. Thanks, Ilya
On 12/8/23 19:58, Ilya Dryomov wrote: > On Fri, Dec 8, 2023 at 5:34 AM <xiubli@redhat.com> wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> The messages from ceph maybe split into multiple socket packages >> and we just need to wait for all the data to be availiable on the >> sokcet. >> >> URL: https://tracker.ceph.com/issues/63586 >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> net/ceph/messenger_v1.c | 18 ++++++++++-------- >> 1 file changed, 10 insertions(+), 8 deletions(-) >> >> diff --git a/net/ceph/messenger_v1.c b/net/ceph/messenger_v1.c >> index f9a50d7f0d20..aff81fef932f 100644 >> --- a/net/ceph/messenger_v1.c >> +++ b/net/ceph/messenger_v1.c >> @@ -1160,15 +1160,17 @@ static int read_partial_message(struct ceph_connection *con) >> /* header */ >> size = sizeof(con->v1.in_hdr); >> end = size; >> - ret = read_partial(con, end, size, &con->v1.in_hdr); >> - if (ret <= 0) >> - return ret; >> + if (con->v1.in_base_pos < end) { >> + ret = read_partial(con, end, size, &con->v1.in_hdr); >> + if (ret <= 0) >> + return ret; >> >> - crc = crc32c(0, &con->v1.in_hdr, offsetof(struct ceph_msg_header, crc)); >> - if (cpu_to_le32(crc) != con->v1.in_hdr.crc) { >> - pr_err("read_partial_message bad hdr crc %u != expected %u\n", >> - crc, con->v1.in_hdr.crc); >> - return -EBADMSG; >> + crc = crc32c(0, &con->v1.in_hdr, offsetof(struct ceph_msg_header, crc)); >> + if (cpu_to_le32(crc) != con->v1.in_hdr.crc) { >> + pr_err("read_partial_message bad hdr crc %u != expected %u\n", >> + crc, con->v1.in_hdr.crc); >> + return -EBADMSG; >> + } >> } >> >> front_len = le32_to_cpu(con->v1.in_hdr.front_len); >> -- >> 2.43.0 >> > Hi Xiubo, > > This doesn't seem right to me. read_partial() is supposed to be called > unconditionally. On a short read (i.e. when it's unable to fill the > destination buffer -- in this case the header), it returns 0 and the > stack is supposed to unroll all the way up to ceph_con_workfn(). > > If the destination buffer is already filled, read_partial() does > nothing and returns 1. Recomputing the header crc in case > read_partial_message() is called repeatedly shouldn't be an issue > because con->v1.in_hdr shouldn't be modified in the interim. If it > gets modified, it's a bug. > > It might help if you provide a step-by-step breakdown of the scenario > that you are trying to address in the commit message. Yeah, you are correct. So my first change was correct, I just adjust the patch before sending it out. Let me change it back. Thanks - Xiubo > Thanks, > > Ilya >
diff --git a/net/ceph/messenger_v1.c b/net/ceph/messenger_v1.c index f9a50d7f0d20..aff81fef932f 100644 --- a/net/ceph/messenger_v1.c +++ b/net/ceph/messenger_v1.c @@ -1160,15 +1160,17 @@ static int read_partial_message(struct ceph_connection *con) /* header */ size = sizeof(con->v1.in_hdr); end = size; - ret = read_partial(con, end, size, &con->v1.in_hdr); - if (ret <= 0) - return ret; + if (con->v1.in_base_pos < end) { + ret = read_partial(con, end, size, &con->v1.in_hdr); + if (ret <= 0) + return ret; - crc = crc32c(0, &con->v1.in_hdr, offsetof(struct ceph_msg_header, crc)); - if (cpu_to_le32(crc) != con->v1.in_hdr.crc) { - pr_err("read_partial_message bad hdr crc %u != expected %u\n", - crc, con->v1.in_hdr.crc); - return -EBADMSG; + crc = crc32c(0, &con->v1.in_hdr, offsetof(struct ceph_msg_header, crc)); + if (cpu_to_le32(crc) != con->v1.in_hdr.crc) { + pr_err("read_partial_message bad hdr crc %u != expected %u\n", + crc, con->v1.in_hdr.crc); + return -EBADMSG; + } } front_len = le32_to_cpu(con->v1.in_hdr.front_len);