* [Virtio-fs] [PATCH] virtiofsd: fix double fuse_mbuf_iter_advance when do_removemapping
@ 2019-12-05 9:01 Catherine Ho
2019-12-05 9:19 ` Peng Tao
0 siblings, 1 reply; 5+ messages in thread
From: Catherine Ho @ 2019-12-05 9:01 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Stefan Hajnoczi, virtio-fs; +Cc: Catherine Ho
Commit 8e92b1fc98f7 ("DAX: virtiofsd: make FUSE_REMOVEMAPPING support
multiple entries") forgot to remove one fuse_mbuf_iter_advance in
do_removemapping.
Without this patch, virtiofsd will report:
[ID: 00000123] do_removemapping: invalid in, expected 1 * 16, has 60 - 60
[ID: 00000123] unique: 232, error: -22 (Invalid argument), outsize: 16
Fixes: 8e92b1fc98f7 ("DAX: virtiofsd: make FUSE_REMOVEMAPPING support multiple entries")
Cc: Peng Tao <tao.peng@linux.alibaba.com>
Signed-off-by: Catherine Ho <catherine.hecx@gmail.com>
---
contrib/virtiofsd/fuse_lowlevel.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
index bde66c72c3..752e7d97b9 100644
--- a/contrib/virtiofsd/fuse_lowlevel.c
+++ b/contrib/virtiofsd/fuse_lowlevel.c
@@ -1894,14 +1894,6 @@ static void do_removemapping(fuse_req_t req, fuse_ino_t nodeid,
return;
}
- one = fuse_mbuf_iter_advance(iter, sizeof(*one));
- if (!one) {
- fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid in, expected %d * %ld, has %ld - %ld\n",
- arg->count, sizeof(*one), iter->size, iter->pos);
- fuse_reply_err(req, EINVAL);
- return;
- }
-
if (req->se->op.removemapping)
req->se->op.removemapping(req, req->se, nodeid, arg->count, one);
else
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: fix double fuse_mbuf_iter_advance when do_removemapping
2019-12-05 9:01 [Virtio-fs] [PATCH] virtiofsd: fix double fuse_mbuf_iter_advance when do_removemapping Catherine Ho
@ 2019-12-05 9:19 ` Peng Tao
2019-12-05 9:34 ` Catherine Ho
0 siblings, 1 reply; 5+ messages in thread
From: Peng Tao @ 2019-12-05 9:19 UTC (permalink / raw)
To: Catherine Ho, Dr. David Alan Gilbert, Stefan Hajnoczi, virtio-fs
On 2019/12/5 17:01, Catherine Ho wrote:
> Commit 8e92b1fc98f7 ("DAX: virtiofsd: make FUSE_REMOVEMAPPING support
> multiple entries") forgot to remove one fuse_mbuf_iter_advance in
> do_removemapping.
No, we do need to advance twice. One for fuse_removemapping_in, another
for an array of fuse_removemapping_one.
>
> Without this patch, virtiofsd will report:
> [ID: 00000123] do_removemapping: invalid in, expected 1 * 16, has 60 - 60
> [ID: 00000123] unique: 232, error: -22 (Invalid argument), outsize: 16
>
What kernel version are you using? It appears that the remove mapping
request does not container a proper fuse_removemapping_one struct.
> Fixes: 8e92b1fc98f7 ("DAX: virtiofsd: make FUSE_REMOVEMAPPING support multiple entries")
> Cc: Peng Tao <tao.peng@linux.alibaba.com>
> Signed-off-by: Catherine Ho <catherine.hecx@gmail.com>
> ---
> contrib/virtiofsd/fuse_lowlevel.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
> index bde66c72c3..752e7d97b9 100644
> --- a/contrib/virtiofsd/fuse_lowlevel.c
> +++ b/contrib/virtiofsd/fuse_lowlevel.c
> @@ -1894,14 +1894,6 @@ static void do_removemapping(fuse_req_t req, fuse_ino_t nodeid,
> return;
> }
>
> - one = fuse_mbuf_iter_advance(iter, sizeof(*one));
> - if (!one) {
> - fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid in, expected %d * %ld, has %ld - %ld\n",
> - arg->count, sizeof(*one), iter->size, iter->pos);
> - fuse_reply_err(req, EINVAL);
> - return;
> - }
> -
> if (req->se->op.removemapping)
> req->se->op.removemapping(req, req->se, nodeid, arg->count, one);
The patch is wrong itself, as `one` is not assigned here.
Cheers,
Tao
--
Into something rich and strange.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: fix double fuse_mbuf_iter_advance when do_removemapping
2019-12-05 9:19 ` Peng Tao
@ 2019-12-05 9:34 ` Catherine Ho
2019-12-05 10:04 ` Peng Tao
0 siblings, 1 reply; 5+ messages in thread
From: Catherine Ho @ 2019-12-05 9:34 UTC (permalink / raw)
To: Peng Tao; +Cc: virtio-fs
Hi Peng
On Thu, 5 Dec 2019 at 17:19, Peng Tao <tao.peng@linux.alibaba.com> wrote:
>
> On 2019/12/5 17:01, Catherine Ho wrote:
> > Commit 8e92b1fc98f7 ("DAX: virtiofsd: make FUSE_REMOVEMAPPING support
> > multiple entries") forgot to remove one fuse_mbuf_iter_advance in
> > do_removemapping.
> No, we do need to advance twice. One for fuse_removemapping_in, another
> for an array of fuse_removemapping_one.
>
But seems it advances for 3 times. What I mean is the 2nd advance of *one*
is pointless.
Please see [1] [2]
[1]https://gitlab.com/virtio-fs/qemu/blob/virtio-fs-dev/contrib/virtiofsd/fuse_lowlevel.c#L1888
[2]https://gitlab.com/virtio-fs/qemu/blob/virtio-fs-dev/contrib/virtiofsd/fuse_lowlevel.c#L1896
Best Regards
Catherine
> >
> > Without this patch, virtiofsd will report:
> > [ID: 00000123] do_removemapping: invalid in, expected 1 * 16, has 60 - 60
> > [ID: 00000123] unique: 232, error: -22 (Invalid argument), outsize: 16
> >
> What kernel version are you using? It appears that the remove mapping
> request does not container a proper fuse_removemapping_one struct.
>
> > Fixes: 8e92b1fc98f7 ("DAX: virtiofsd: make FUSE_REMOVEMAPPING support multiple entries")
> > Cc: Peng Tao <tao.peng@linux.alibaba.com>
> > Signed-off-by: Catherine Ho <catherine.hecx@gmail.com>
> > ---
> > contrib/virtiofsd/fuse_lowlevel.c | 8 --------
> > 1 file changed, 8 deletions(-)
> >
> > diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
> > index bde66c72c3..752e7d97b9 100644
> > --- a/contrib/virtiofsd/fuse_lowlevel.c
> > +++ b/contrib/virtiofsd/fuse_lowlevel.c
> > @@ -1894,14 +1894,6 @@ static void do_removemapping(fuse_req_t req, fuse_ino_t nodeid,
> > return;
> > }
> >
> > - one = fuse_mbuf_iter_advance(iter, sizeof(*one));
> > - if (!one) {
> > - fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid in, expected %d * %ld, has %ld - %ld\n",
> > - arg->count, sizeof(*one), iter->size, iter->pos);
> > - fuse_reply_err(req, EINVAL);
> > - return;
> > - }
> > -
> > if (req->se->op.removemapping)
> > req->se->op.removemapping(req, req->se, nodeid, arg->count, one);
> The patch is wrong itself, as `one` is not assigned here.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: fix double fuse_mbuf_iter_advance when do_removemapping
2019-12-05 9:34 ` Catherine Ho
@ 2019-12-05 10:04 ` Peng Tao
2019-12-05 10:41 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 5+ messages in thread
From: Peng Tao @ 2019-12-05 10:04 UTC (permalink / raw)
To: Catherine Ho; +Cc: virtio-fs
On 2019/12/5 17:34, Catherine Ho wrote:
> Hi Peng
>
> On Thu, 5 Dec 2019 at 17:19, Peng Tao <tao.peng@linux.alibaba.com> wrote:
>>
>> On 2019/12/5 17:01, Catherine Ho wrote:
>>> Commit 8e92b1fc98f7 ("DAX: virtiofsd: make FUSE_REMOVEMAPPING support
>>> multiple entries") forgot to remove one fuse_mbuf_iter_advance in
>>> do_removemapping.
>> No, we do need to advance twice. One for fuse_removemapping_in, another
>> for an array of fuse_removemapping_one.
>>
> But seems it advances for 3 times. What I mean is the 2nd advance of *one*
> is pointless.
> Please see [1] [2]
> [1]https://gitlab.com/virtio-fs/qemu/blob/virtio-fs-dev/contrib/virtiofsd/fuse_lowlevel.c#L1888
> [2]https://gitlab.com/virtio-fs/qemu/blob/virtio-fs-dev/contrib/virtiofsd/fuse_lowlevel.c#L1896
>
oops, that does look like a merge hunk. I don't have it in my local
branch. After pulling the latest code I can see it too. And there was a
branch reset at some point.
From https://gitlab.com/virtio-fs/qemu
+ 42db960...5a356e6 virtio-fs-dev -> virtiofs/virtio-fs-dev (forced
update)
Now your patch looks good to me. Thanks!
Cheers,
Tao
> Best Regards
> Catherine
>>>
>>> Without this patch, virtiofsd will report:
>>> [ID: 00000123] do_removemapping: invalid in, expected 1 * 16, has 60 - 60
>>> [ID: 00000123] unique: 232, error: -22 (Invalid argument), outsize: 16
>>>
>> What kernel version are you using? It appears that the remove mapping
>> request does not container a proper fuse_removemapping_one struct.
>>
>>> Fixes: 8e92b1fc98f7 ("DAX: virtiofsd: make FUSE_REMOVEMAPPING support multiple entries")
>>> Cc: Peng Tao <tao.peng@linux.alibaba.com>
>>> Signed-off-by: Catherine Ho <catherine.hecx@gmail.com>
Reviewed-by: Peng Tao <tao.peng@linux.alibaba.com>
>>> ---
>>> contrib/virtiofsd/fuse_lowlevel.c | 8 --------
>>> 1 file changed, 8 deletions(-)
>>>
>>> diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
>>> index bde66c72c3..752e7d97b9 100644
>>> --- a/contrib/virtiofsd/fuse_lowlevel.c
>>> +++ b/contrib/virtiofsd/fuse_lowlevel.c
>>> @@ -1894,14 +1894,6 @@ static void do_removemapping(fuse_req_t req, fuse_ino_t nodeid,
>>> return;
>>> }
>>>
>>> - one = fuse_mbuf_iter_advance(iter, sizeof(*one));
>>> - if (!one) {
>>> - fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid in, expected %d * %ld, has %ld - %ld\n",
>>> - arg->count, sizeof(*one), iter->size, iter->pos);
>>> - fuse_reply_err(req, EINVAL);
>>> - return;
>>> - }
>>> -
>>> if (req->se->op.removemapping)
>>> req->se->op.removemapping(req, req->se, nodeid, arg->count, one);
>> The patch is wrong itself, as `one` is not assigned here.
>>
--
Into something rich and strange.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: fix double fuse_mbuf_iter_advance when do_removemapping
2019-12-05 10:04 ` Peng Tao
@ 2019-12-05 10:41 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 5+ messages in thread
From: Dr. David Alan Gilbert @ 2019-12-05 10:41 UTC (permalink / raw)
To: Peng Tao; +Cc: virtio-fs, Catherine Ho
* Peng Tao (tao.peng@linux.alibaba.com) wrote:
>
>
> On 2019/12/5 17:34, Catherine Ho wrote:
> > Hi Peng
> >
> > On Thu, 5 Dec 2019 at 17:19, Peng Tao <tao.peng@linux.alibaba.com> wrote:
> > >
> > > On 2019/12/5 17:01, Catherine Ho wrote:
> > > > Commit 8e92b1fc98f7 ("DAX: virtiofsd: make FUSE_REMOVEMAPPING support
> > > > multiple entries") forgot to remove one fuse_mbuf_iter_advance in
> > > > do_removemapping.
> > > No, we do need to advance twice. One for fuse_removemapping_in, another
> > > for an array of fuse_removemapping_one.
> > >
> > But seems it advances for 3 times. What I mean is the 2nd advance of *one*
> > is pointless.
> > Please see [1] [2]
> > [1]https://gitlab.com/virtio-fs/qemu/blob/virtio-fs-dev/contrib/virtiofsd/fuse_lowlevel.c#L1888
> > [2]https://gitlab.com/virtio-fs/qemu/blob/virtio-fs-dev/contrib/virtiofsd/fuse_lowlevel.c#L1896
> >
> oops, that does look like a merge hunk. I don't have it in my local branch.
> After pulling the latest code I can see it too. And there was a branch reset
> at some point.
>
> From https://gitlab.com/virtio-fs/qemu
> + 42db960...5a356e6 virtio-fs-dev -> virtiofs/virtio-fs-dev (forced
> update)
>
> Now your patch looks good to me. Thanks!
That's probably my screwup sometime during a rebase.
Catherine: Thanks; I'll find the right place to merge your fix in.
Dave
> Cheers,
> Tao
> > Best Regards
> > Catherine
> > > >
> > > > Without this patch, virtiofsd will report:
> > > > [ID: 00000123] do_removemapping: invalid in, expected 1 * 16, has 60 - 60
> > > > [ID: 00000123] unique: 232, error: -22 (Invalid argument), outsize: 16
> > > >
> > > What kernel version are you using? It appears that the remove mapping
> > > request does not container a proper fuse_removemapping_one struct.
> > >
> > > > Fixes: 8e92b1fc98f7 ("DAX: virtiofsd: make FUSE_REMOVEMAPPING support multiple entries")
> > > > Cc: Peng Tao <tao.peng@linux.alibaba.com>
> > > > Signed-off-by: Catherine Ho <catherine.hecx@gmail.com>
> Reviewed-by: Peng Tao <tao.peng@linux.alibaba.com>
>
> > > > ---
> > > > contrib/virtiofsd/fuse_lowlevel.c | 8 --------
> > > > 1 file changed, 8 deletions(-)
> > > >
> > > > diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
> > > > index bde66c72c3..752e7d97b9 100644
> > > > --- a/contrib/virtiofsd/fuse_lowlevel.c
> > > > +++ b/contrib/virtiofsd/fuse_lowlevel.c
> > > > @@ -1894,14 +1894,6 @@ static void do_removemapping(fuse_req_t req, fuse_ino_t nodeid,
> > > > return;
> > > > }
> > > >
> > > > - one = fuse_mbuf_iter_advance(iter, sizeof(*one));
> > > > - if (!one) {
> > > > - fuse_log(FUSE_LOG_ERR, "do_removemapping: invalid in, expected %d * %ld, has %ld - %ld\n",
> > > > - arg->count, sizeof(*one), iter->size, iter->pos);
> > > > - fuse_reply_err(req, EINVAL);
> > > > - return;
> > > > - }
> > > > -
> > > > if (req->se->op.removemapping)
> > > > req->se->op.removemapping(req, req->se, nodeid, arg->count, one);
> > > The patch is wrong itself, as `one` is not assigned here.
> > >
>
> --
> Into something rich and strange.
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-05 10:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-05 9:01 [Virtio-fs] [PATCH] virtiofsd: fix double fuse_mbuf_iter_advance when do_removemapping Catherine Ho
2019-12-05 9:19 ` Peng Tao
2019-12-05 9:34 ` Catherine Ho
2019-12-05 10:04 ` Peng Tao
2019-12-05 10:41 ` Dr. David Alan Gilbert
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.