linux-kernel - Re: [PATCH v2] vfs: fix bug when opening a file with O_DIRECT on a file system that does not support it will leave an empty file

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

Date:   Tue, 4 Jan 2022 21:40:20 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Qinghua Jin <qhjin.dev@...il.com>
Cc:     Colin Ian King <colin.king@...onical.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] vfs: fix bug when opening a file with O_DIRECT on a
 file system that does not support it will leave an empty file

On Wed, Jan 05, 2022 at 05:23:30AM +0800, Qinghua Jin wrote:
> OK, thanks.
> So, how to resolve this bug? I think we should check the param before
> creating the inode and dentry. Maybe the best place is in the lookup_open
> where the last component get created, and the parent inode and child inode
> is in the same filesystem, so I think we can use
> dir_inode->i_mapping->a_ops->direct_IO to check the O_DIRECT param. The
> code looks like this:
>   if((open_flag & O_CREAT) && (open_flag & O_DIRECT)) {
>     if (!dir_inode->i_mapping || !dir_inode->i_mapping->a_ops ||
>             !dir_inode->i_mapping->a_ops->direct_IO)
>                 return ERR_PTR(-EINVAL);
>     }
> should do the work.

Why would it?  Seriously, why would directories' use of page cache resemble that
by the regular files on the same fs?  Sure, for minixfs we have that - directory
contents there happens to be stored in exact same way as for regular files.
For many other filesystem types it's not true.

Incidentally, how would an inode possibly get NULL ->i_mapping?  Or NULL
->a_ops, for that matter (IOW, the existing check is also partially
cargo-culted)...