linux-kernel - Re: [PATCH] net: 9p: free what was emitted when read count is 0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 Mar 2021 15:39:40 +0800
From:   Jisheng Zhang <Jisheng.Zhang@...aptics.com>
To:     Dominique Martinet <asmadeus@...ewreck.org>
Cc:     Eric Van Hensbergen <ericvh@...il.com>,
        Latchesar Ionkov <lucho@...kov.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        v9fs-developer@...ts.sourceforge.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: 9p: free what was emitted when read count is 0

On Tue, 2 Mar 2021 13:38:08 +0900 Dominique Martinet wrote:

> 
> 
> Jisheng Zhang wrote on Mon, Mar 01, 2021 at 11:01:57AM +0800:
> > Per my understanding of iov_iter, we need to call iov_iter_advance()
> > even when the read out count is 0. I believe we can see this common style
> > in other fs.  
> 
> I'm not sure where you see this style, but I don't see exceptions for
> 0-sized read not advancing the iov in general, and I guess this makes
> sense.

for example, function dio_refill_pages() in fs/direct-io.c, and below code piece
from net/core/datagram.c:

                copied = iov_iter_get_pages(from, pages, length,
                                            MAX_SKB_FRAGS - frag, &start);
                if (copied < 0)
                        return -EFAULT;

                iov_iter_advance(from, copied);

As can be seen, for "copied >=0" case, we call iov_iter_advance()

> 
> 
> Rather than make an exception for 0, how about just removing the if as
> follow ?

IMHO, we may need to keep the "if" in current logic. When count
reaches zero, we need to break the "while(iov_iter_count(to))" loop, so removing
the "if" modifying the logic.

> 
> I've checked that the non_zc case (copy_to_iter with 0 size) also works
> to the same effect, so I'm not sure why the check got added in the
> first place... But then again this is old code so maybe the semantics
> changed since 2015.
> 
> 
> ----
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 4f62f299da0c..0a0039255c5b 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -1623,11 +1623,6 @@ p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
>         }
> 
>         p9_debug(P9_DEBUG_9P, "<<< RREAD count %d\n", count);
> -       if (!count) {
> -               p9_tag_remove(clnt, req);
> -               return 0;
> -       }
> -
>         if (non_zc) {
>                 int n = copy_to_iter(dataptr, count, to);
> 
> 
> ----
> 
> If you're ok with that, would you mind resending that way?
> 
> I'd also want the commit message to be reworded a bit, at least the
> first line (summary) doesn't make sense right now: I have no idea
> what you mean by "free what was emitted".
> Just "9p: advance iov on empty read" or something similar would do.

Thanks for the suggestion. I will send a v2 to update the commit msg but
keep the patch as is if you agree with above keeping "if" logic.
> 
> 
> > > cat version? coreutils' doesn't seem to do that on their git)  
> >
> > busybox cat  
> 
> Ok, could reproduce with busybox cat, thanks.
> As expected I can't reproduce with older kernels so will run a bisect
> for the sake of it as time allows
> 

Thanks