linux-ext4 - Re: [PATCH v3 5/6] ext4: introduce direct IO write path using iomap infrastructure

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]

Message-ID: <20190916101417.GA4024@bobrowski>
Date:   Mon, 16 Sep 2019 20:14:17 +1000
From:   Matthew Bobrowski <mbobrowski@...browski.org>
To:     Ritesh Harjani <riteshh@...ux.ibm.com>
Cc:     tytso@....edu, jack@...e.cz, adilger.kernel@...ger.ca,
        linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        david@...morbit.com, hch@...radead.org, darrick.wong@...cle.com
Subject: Re: [PATCH v3 5/6] ext4: introduce direct IO write path using iomap
 infrastructure

On Mon, Sep 16, 2019 at 10:07:41AM +0530, Ritesh Harjani wrote:
> > @@ -213,12 +214,16 @@ static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from)
> >   	struct inode *inode = file_inode(iocb->ki_filp);
> >   	ssize_t ret;
> > 
> > +	if (unlikely(IS_IMMUTABLE(inode)))
> > +		return -EPERM;
> > +
> >   	ret = generic_write_checks(iocb, from);
> >   	if (ret <= 0)
> >   		return ret;
> > 
> > -	if (unlikely(IS_IMMUTABLE(inode)))
> > -		return -EPERM;
> > +	ret = file_modified(iocb->ki_filp);
> > +	if (ret)
> > +		return 0;
> 
> Why not return ret directly, otherwise we will be returning the wrong
> error code to user space. Thoughts?

You're right. I can't remember exactly why I decided to return '0', however
looking at the code once again I don't see a reason why we don't just return
'ret', as any value other than '0' represents a failure in this case
anyway. Thanks for picking that up.
 
> Do you think simplification/restructuring of this API
> "ext4_write_checks" can be a separate patch, so that this patch
> only focuses on conversion of DIO write path to iomap?

Hm, if we split it up so that it comes before this patch then it becomes hairy
in the sense that a whole bunch of other changes would also need to come with
what looks to be such a miniscule modification
i.e. ext4_buffered_write_iter(), ext4_file_write_iter(), etc. Splitting it to
come after just doesn't make any sense. To be honest, I don't really have any
strong opinions around why we shouldn't split it up, nor do I have a strong
opinion around why we should, so I think we should just leave it for now.

> Also, I think we can make the function (ext4_write_checks())
> like below. This way we call for file_modified() only after we
> have checked for write limits, at one place.

No objections and I think it's a good idea.

>   static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter
> *from)
>   {
>           struct inode *inode = file_inode(iocb->ki_filp);
>           ssize_t ret;
> 
>           if (unlikely(IS_IMMUTABLE(inode)))
>                   return -EPERM;
> 
>           ret = generic_write_checks(iocb, from);
>           if (ret <= 0)
> _                 return ret;
>           /*
>            * If we have encountered a bitmap-format file, the size limit
>            * is smaller than s_maxbytes, which is for extent-mapped files.
>            */
>           if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
>                   struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> 
>                   if (iocb->ki_pos >= sbi->s_bitmap_maxbytes)
>                           return -EFBIG;
>                   iov_iter_truncate(from, sbi->s_bitmap_maxbytes -
> iocb->ki_pos);
>           }
> +
> +         ret = file_modified(iocb->ki_filp);
> +         if (ret)
> +                 return ret;
> +
>           return iov_iter_count(from);
>   }

--<M>--