On Thu, May 28, 2020 at 06:27:36PM -0700, Linus Torvalds wrote:
> On Thu, May 28, 2020 at 5:04 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > if (*ppos >= i_size_read(inode))
> > return 0;
> >
> > + /* don't read past the lvb */
> > + if (count > i_size_read(inode) - *ppos)
> > + count = i_size_read(inode) - *ppos;
>
> This isn't a new problem, since you effectively just moved this code,
> but it's perhaps more obvious now..
>
> "i_size_read()" is not necessarily stable - we do special things on
> 32-bit to make sure that we get _a_ stable value for it, but it's not
> necessarily guaranteed to be the same value when called twice. Think
> concurrent pread() with a write..
>
> So the inode size could change in between those two accesses, and the
> subtraction might end up underflowing despite the check just above.
>
> This might not be an issue with ocfs2 (I didn't check locking), but ..
case S_IFREG:
inode->i_op = &dlmfs_file_inode_operations;
inode->i_fop = &dlmfs_file_operations;
i_size_write(inode, DLM_LVB_LEN);
is the only thing that does anything to size of that sucker. IOW, that
i_size_read() might as well had been an explicit 64. Actually,
looking at that thing I would suggest simply
static ssize_t dlmfs_file_read(struct file *filp,
char __user *buf,
size_t count,
loff_t *ppos)
{
struct inode *inode = file_inode(filp);
char lvb_buf[DLM_LVB_LEN];
if (!user_dlm_read_lvb(inode, lvb_buf, DLM_LVB_LEN))
return 0;
return simple_read_from_buffer(buf, count, ppos,
lvb_buf, DLM_LVB_LEN);
}
But that's belongs in a followup, IMO.
