* [PATCH] nvmet-tcp: fix data digest pointer calculation
@ 2021-10-25 17:16 Varun Prakash
2021-10-26 13:24 ` Sagi Grimberg
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Varun Prakash @ 2021-10-25 17:16 UTC (permalink / raw)
To: sagi, hch; +Cc: linux-nvme, varun
exp_ddgst is of type __le32, &cmd->exp_ddgst + cmd->offset
increases &cmd->exp_ddgst by 4 * cmd->offset, fix this by
type casting &cmd->exp_ddgst to u8 *.
Fixes: 872d26a391da ("nvmet-tcp: add NVMe over TCP target driver")
Signed-off-by: Varun Prakash <varun@chelsio.com>
---
drivers/nvme/target/tcp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 07ee347..d641bfa 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -702,7 +702,7 @@ static int nvmet_try_send_ddgst(struct nvmet_tcp_cmd *cmd, bool last_in_batch)
struct nvmet_tcp_queue *queue = cmd->queue;
struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
struct kvec iov = {
- .iov_base = &cmd->exp_ddgst + cmd->offset,
+ .iov_base = (u8 *)&cmd->exp_ddgst + cmd->offset,
.iov_len = NVME_TCP_DIGEST_LENGTH - cmd->offset
};
int ret;
--
2.0.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] nvmet-tcp: fix data digest pointer calculation
2021-10-25 17:16 [PATCH] nvmet-tcp: fix data digest pointer calculation Varun Prakash
@ 2021-10-26 13:24 ` Sagi Grimberg
2021-10-27 5:57 ` Christoph Hellwig
2021-10-27 21:37 ` Keith Busch
2 siblings, 0 replies; 12+ messages in thread
From: Sagi Grimberg @ 2021-10-26 13:24 UTC (permalink / raw)
To: Varun Prakash, hch; +Cc: linux-nvme
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvmet-tcp: fix data digest pointer calculation
2021-10-25 17:16 [PATCH] nvmet-tcp: fix data digest pointer calculation Varun Prakash
2021-10-26 13:24 ` Sagi Grimberg
@ 2021-10-27 5:57 ` Christoph Hellwig
2021-10-27 7:11 ` Sagi Grimberg
2021-10-27 21:37 ` Keith Busch
2 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2021-10-27 5:57 UTC (permalink / raw)
To: Varun Prakash; +Cc: sagi, hch, linux-nvme
On Mon, Oct 25, 2021 at 10:46:54PM +0530, Varun Prakash wrote:
> exp_ddgst is of type __le32, &cmd->exp_ddgst + cmd->offset
> increases &cmd->exp_ddgst by 4 * cmd->offset, fix this by
> type casting &cmd->exp_ddgst to u8 *.
>
> Fixes: 872d26a391da ("nvmet-tcp: add NVMe over TCP target driver")
> Signed-off-by: Varun Prakash <varun@chelsio.com>
> ---
> drivers/nvme/target/tcp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index 07ee347..d641bfa 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -702,7 +702,7 @@ static int nvmet_try_send_ddgst(struct nvmet_tcp_cmd *cmd, bool last_in_batch)
> struct nvmet_tcp_queue *queue = cmd->queue;
> struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
> struct kvec iov = {
> - .iov_base = &cmd->exp_ddgst + cmd->offset,
> + .iov_base = (u8 *)&cmd->exp_ddgst + cmd->offset,
Wouldn't be the better fix to divide cmd->offset by 4 instead of the
casts? I can fix this up inline if that is ok.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvmet-tcp: fix data digest pointer calculation
2021-10-27 5:57 ` Christoph Hellwig
@ 2021-10-27 7:11 ` Sagi Grimberg
2021-10-27 7:13 ` Christoph Hellwig
0 siblings, 1 reply; 12+ messages in thread
From: Sagi Grimberg @ 2021-10-27 7:11 UTC (permalink / raw)
To: Christoph Hellwig, Varun Prakash; +Cc: linux-nvme
>> exp_ddgst is of type __le32, &cmd->exp_ddgst + cmd->offset
>> increases &cmd->exp_ddgst by 4 * cmd->offset, fix this by
>> type casting &cmd->exp_ddgst to u8 *.
>>
>> Fixes: 872d26a391da ("nvmet-tcp: add NVMe over TCP target driver")
>> Signed-off-by: Varun Prakash <varun@chelsio.com>
>> ---
>> drivers/nvme/target/tcp.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
>> index 07ee347..d641bfa 100644
>> --- a/drivers/nvme/target/tcp.c
>> +++ b/drivers/nvme/target/tcp.c
>> @@ -702,7 +702,7 @@ static int nvmet_try_send_ddgst(struct nvmet_tcp_cmd *cmd, bool last_in_batch)
>> struct nvmet_tcp_queue *queue = cmd->queue;
>> struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
>> struct kvec iov = {
>> - .iov_base = &cmd->exp_ddgst + cmd->offset,
>> + .iov_base = (u8 *)&cmd->exp_ddgst + cmd->offset,
>
> Wouldn't be the better fix to divide cmd->offset by 4 instead of the
> casts? I can fix this up inline if that is ok.
Don't really mind, what makes it a better fix?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvmet-tcp: fix data digest pointer calculation
2021-10-27 7:11 ` Sagi Grimberg
@ 2021-10-27 7:13 ` Christoph Hellwig
2021-10-27 9:49 ` Sagi Grimberg
0 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2021-10-27 7:13 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Varun Prakash, linux-nvme
On Wed, Oct 27, 2021 at 10:11:01AM +0300, Sagi Grimberg wrote:
>>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
>>> index 07ee347..d641bfa 100644
>>> --- a/drivers/nvme/target/tcp.c
>>> +++ b/drivers/nvme/target/tcp.c
>>> @@ -702,7 +702,7 @@ static int nvmet_try_send_ddgst(struct nvmet_tcp_cmd *cmd, bool last_in_batch)
>>> struct nvmet_tcp_queue *queue = cmd->queue;
>>> struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
>>> struct kvec iov = {
>>> - .iov_base = &cmd->exp_ddgst + cmd->offset,
>>> + .iov_base = (u8 *)&cmd->exp_ddgst + cmd->offset,
>>
>> Wouldn't be the better fix to divide cmd->offset by 4 instead of the
>> casts? I can fix this up inline if that is ok.
>
> Don't really mind, what makes it a better fix?
I generally prefer to avoid casts as they paper over bugs.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvmet-tcp: fix data digest pointer calculation
2021-10-27 7:13 ` Christoph Hellwig
@ 2021-10-27 9:49 ` Sagi Grimberg
2021-10-27 14:49 ` Varun Prakash
0 siblings, 1 reply; 12+ messages in thread
From: Sagi Grimberg @ 2021-10-27 9:49 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Varun Prakash, linux-nvme
>>>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
>>>> index 07ee347..d641bfa 100644
>>>> --- a/drivers/nvme/target/tcp.c
>>>> +++ b/drivers/nvme/target/tcp.c
>>>> @@ -702,7 +702,7 @@ static int nvmet_try_send_ddgst(struct nvmet_tcp_cmd *cmd, bool last_in_batch)
>>>> struct nvmet_tcp_queue *queue = cmd->queue;
>>>> struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
>>>> struct kvec iov = {
>>>> - .iov_base = &cmd->exp_ddgst + cmd->offset,
>>>> + .iov_base = (u8 *)&cmd->exp_ddgst + cmd->offset,
>>>
>>> Wouldn't be the better fix to divide cmd->offset by 4 instead of the
>>> casts? I can fix this up inline if that is ok.
>>
>> Don't really mind, what makes it a better fix?
>
> I generally prefer to avoid casts as they paper over bugs.
I'm fine either way.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvmet-tcp: fix data digest pointer calculation
2021-10-27 9:49 ` Sagi Grimberg
@ 2021-10-27 14:49 ` Varun Prakash
2021-10-28 8:26 ` Sagi Grimberg
0 siblings, 1 reply; 12+ messages in thread
From: Varun Prakash @ 2021-10-27 14:49 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, linux-nvme
On Wed, Oct 27, 2021 at 12:49:31PM +0300, Sagi Grimberg wrote:
>
> >>>>diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> >>>>index 07ee347..d641bfa 100644
> >>>>--- a/drivers/nvme/target/tcp.c
> >>>>+++ b/drivers/nvme/target/tcp.c
> >>>>@@ -702,7 +702,7 @@ static int nvmet_try_send_ddgst(struct nvmet_tcp_cmd *cmd, bool last_in_batch)
> >>>> struct nvmet_tcp_queue *queue = cmd->queue;
> >>>> struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
> >>>> struct kvec iov = {
> >>>>- .iov_base = &cmd->exp_ddgst + cmd->offset,
> >>>>+ .iov_base = (u8 *)&cmd->exp_ddgst + cmd->offset,
> >>>
> >>>Wouldn't be the better fix to divide cmd->offset by 4 instead of the
> >>>casts? I can fix this up inline if that is ok.
> >>
> >>Don't really mind, what makes it a better fix?
> >
> >I generally prefer to avoid casts as they paper over bugs.
>
> I'm fine either way.
cmd->offset can have value 0,1,2,3. cmd->offset / 4 will always be 0.
If 2 bytes of digest are transmitted in the first call to kernel_sendmsg()
then in the second call remaining 2 bytes has to be transmitted so data digest
buffer pointer should increase by 2 bytes.
There is one more bug in nvmet_try_send_ddgst() it currently does not check
whether digest is send successfully. For successful digest send
cmd->offset + ret should be equal to NVME_TCP_DIGEST_LENGTH.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvmet-tcp: fix data digest pointer calculation
2021-10-25 17:16 [PATCH] nvmet-tcp: fix data digest pointer calculation Varun Prakash
2021-10-26 13:24 ` Sagi Grimberg
2021-10-27 5:57 ` Christoph Hellwig
@ 2021-10-27 21:37 ` Keith Busch
2 siblings, 0 replies; 12+ messages in thread
From: Keith Busch @ 2021-10-27 21:37 UTC (permalink / raw)
To: Varun Prakash; +Cc: sagi, hch, linux-nvme
On Mon, Oct 25, 2021 at 10:46:54PM +0530, Varun Prakash wrote:
> @@ -702,7 +702,7 @@ static int nvmet_try_send_ddgst(struct nvmet_tcp_cmd *cmd, bool last_in_batch)
> struct nvmet_tcp_queue *queue = cmd->queue;
> struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
> struct kvec iov = {
> - .iov_base = &cmd->exp_ddgst + cmd->offset,
> + .iov_base = (u8 *)&cmd->exp_ddgst + cmd->offset,
This certainly works too (and it's already applied anyway), though
everywhere else in this driver casts these to 'void *'.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvmet-tcp: fix data digest pointer calculation
2021-10-27 14:49 ` Varun Prakash
@ 2021-10-28 8:26 ` Sagi Grimberg
2021-10-28 10:49 ` Varun Prakash
0 siblings, 1 reply; 12+ messages in thread
From: Sagi Grimberg @ 2021-10-28 8:26 UTC (permalink / raw)
To: Varun Prakash; +Cc: Christoph Hellwig, linux-nvme
>>>>>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
>>>>>> index 07ee347..d641bfa 100644
>>>>>> --- a/drivers/nvme/target/tcp.c
>>>>>> +++ b/drivers/nvme/target/tcp.c
>>>>>> @@ -702,7 +702,7 @@ static int nvmet_try_send_ddgst(struct nvmet_tcp_cmd *cmd, bool last_in_batch)
>>>>>> struct nvmet_tcp_queue *queue = cmd->queue;
>>>>>> struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
>>>>>> struct kvec iov = {
>>>>>> - .iov_base = &cmd->exp_ddgst + cmd->offset,
>>>>>> + .iov_base = (u8 *)&cmd->exp_ddgst + cmd->offset,
>>>>>
>>>>> Wouldn't be the better fix to divide cmd->offset by 4 instead of the
>>>>> casts? I can fix this up inline if that is ok.
>>>>
>>>> Don't really mind, what makes it a better fix?
>>>
>>> I generally prefer to avoid casts as they paper over bugs.
>>
>> I'm fine either way.
>
> cmd->offset can have value 0,1,2,3. cmd->offset / 4 will always be 0.
> If 2 bytes of digest are transmitted in the first call to kernel_sendmsg()
> then in the second call remaining 2 bytes has to be transmitted so data digest
> buffer pointer should increase by 2 bytes.
Right, so let's just change the cast to (void *). Varun, can you respin
the patches?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvmet-tcp: fix data digest pointer calculation
2021-10-28 8:26 ` Sagi Grimberg
@ 2021-10-28 10:49 ` Varun Prakash
2021-10-28 14:00 ` Christoph Hellwig
0 siblings, 1 reply; 12+ messages in thread
From: Varun Prakash @ 2021-10-28 10:49 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, linux-nvme
On Thu, Oct 28, 2021 at 11:26:02AM +0300, Sagi Grimberg wrote:
>
> >>>>>>diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> >>>>>>index 07ee347..d641bfa 100644
> >>>>>>--- a/drivers/nvme/target/tcp.c
> >>>>>>+++ b/drivers/nvme/target/tcp.c
> >>>>>>@@ -702,7 +702,7 @@ static int nvmet_try_send_ddgst(struct nvmet_tcp_cmd *cmd, bool last_in_batch)
> >>>>>> struct nvmet_tcp_queue *queue = cmd->queue;
> >>>>>> struct msghdr msg = { .msg_flags = MSG_DONTWAIT };
> >>>>>> struct kvec iov = {
> >>>>>>- .iov_base = &cmd->exp_ddgst + cmd->offset,
> >>>>>>+ .iov_base = (u8 *)&cmd->exp_ddgst + cmd->offset,
> >>>>>
> >>>>>Wouldn't be the better fix to divide cmd->offset by 4 instead of the
> >>>>>casts? I can fix this up inline if that is ok.
> >>>>
> >>>>Don't really mind, what makes it a better fix?
> >>>
> >>>I generally prefer to avoid casts as they paper over bugs.
> >>
> >>I'm fine either way.
> >
> >cmd->offset can have value 0,1,2,3. cmd->offset / 4 will always be 0.
> >If 2 bytes of digest are transmitted in the first call to kernel_sendmsg()
> >then in the second call remaining 2 bytes has to be transmitted so data digest
> >buffer pointer should increase by 2 bytes.
>
> Right, so let's just change the cast to (void *). Varun, can you respin
> the patches?
Both the patches are already applied to nvme-5.15, should I send v2 or
create a new patch for u8 * -> void * change?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvmet-tcp: fix data digest pointer calculation
2021-10-28 10:49 ` Varun Prakash
@ 2021-10-28 14:00 ` Christoph Hellwig
2021-10-28 14:07 ` Sagi Grimberg
0 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2021-10-28 14:00 UTC (permalink / raw)
To: Varun Prakash; +Cc: Sagi Grimberg, Christoph Hellwig, linux-nvme
On Thu, Oct 28, 2021 at 04:19:48PM +0530, Varun Prakash wrote:
> Both the patches are already applied to nvme-5.15, should I send v2 or
> create a new patch for u8 * -> void * change?
I'm about to send the pull request with them to Jens. It is too late
in the cycle now to respin again.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nvmet-tcp: fix data digest pointer calculation
2021-10-28 14:00 ` Christoph Hellwig
@ 2021-10-28 14:07 ` Sagi Grimberg
0 siblings, 0 replies; 12+ messages in thread
From: Sagi Grimberg @ 2021-10-28 14:07 UTC (permalink / raw)
To: Christoph Hellwig, Varun Prakash; +Cc: linux-nvme
>> Both the patches are already applied to nvme-5.15, should I send v2 or
>> create a new patch for u8 * -> void * change?
>
> I'm about to send the pull request with them to Jens. It is too late
> in the cycle now to respin again.
We can take care of that later...
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-10-28 14:07 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-25 17:16 [PATCH] nvmet-tcp: fix data digest pointer calculation Varun Prakash
2021-10-26 13:24 ` Sagi Grimberg
2021-10-27 5:57 ` Christoph Hellwig
2021-10-27 7:11 ` Sagi Grimberg
2021-10-27 7:13 ` Christoph Hellwig
2021-10-27 9:49 ` Sagi Grimberg
2021-10-27 14:49 ` Varun Prakash
2021-10-28 8:26 ` Sagi Grimberg
2021-10-28 10:49 ` Varun Prakash
2021-10-28 14:00 ` Christoph Hellwig
2021-10-28 14:07 ` Sagi Grimberg
2021-10-27 21:37 ` Keith Busch
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.