linux-kernel - Re: [PATCH] fs: btrfs: Change return type to vm_fault

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] [day] [month] [year] [list]

Message-ID: <20180606144535.GH3539@twin.jikos.cz>
Date:   Wed, 6 Jun 2018 16:45:35 +0200
From:   David Sterba <dsterba@...e.cz>
To:     Souptick Joarder <jrdr.linux@...il.com>
Cc:     dsterba@...e.cz, Matthew Wilcox <willy@...radead.org>,
        Chris Mason <clm@...com>, jbacik@...com, dsterba@...e.com,
        linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fs: btrfs: Change return type to vm_fault_t

On Wed, Jun 06, 2018 at 07:39:53PM +0530, Souptick Joarder wrote:
> >> @@ -8885,8 +8885,9 @@ int btrfs_page_mkwrite(struct vm_fault *vmf)
> >>       char *kaddr;
> >>       unsigned long zero_start;
> >>       loff_t size;
> >> -     int ret;
> >> +     int err;
> >
> > Please use ret2 for the variable name. We're leaving the err/ret naming
> > pattern and use 'ret' to match the function return value and ret2, ret3
> > for the temporary return values.
> 
> Is this new pattern specific to fs/btrfs file system ?

Yes it is, you're not expected to know it in advance but as this was not
the only thing to update in the patch I did not fix it myself.

> >> -     ret = 0;
> >> +     err = 0;
> >>
> >>       /* page is wholly or partially inside EOF */
> >>       if (page_start + PAGE_SIZE > size)
> >> @@ -9009,11 +9007,11 @@ int btrfs_page_mkwrite(struct vm_fault *vmf)
> >>       unlock_extent_cached(io_tree, page_start, page_end, &cached_state);
> >>
> >>  out_unlock:
> >> -     if (!ret) {
> >> +     if (!err) {
> >>               btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, true);
> >>               sb_end_pagefault(inode->i_sb);
> >>               extent_changeset_free(data_reserved);
> >> -             return VM_FAULT_LOCKED;
> >> +             ret = VM_FAULT_LOCKED;
> >
> > That's not a direct change as it changes the code flow and lets the
> > branch continue. As the error handling and cleanup code is partially
> > done in this branch continuing wrong AFAICS.
> 
> Sorry about it, I will correct and send v2.
> Is it possible to get it merge in 4.18 / 4.18-rc-X ?

Ok.