Re: [PATCH] tmpfs: fix uninitialized return value in shmem_link

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 2899 bytes --]

On Mon, Feb 25, 2019 at 12:34 PM Hugh Dickins <hughd@google.com> wrote:
>
> Seems like a gcc bug? But I don't have a decent recent gcc to hand
> to submit a proper report, hope someone else can shed light on it.

I don't have a _very_ recent gcc either, but with gcc-8.2.1 the
attached test-case gives me:

   [torvalds@i7 ~]$ gcc -O2 -S -Wall test.c

with no warning, and then

   [torvalds@i7 ~]$ gcc -O2 -S -Wall -DHIDE_PROBLEM test.c
   test.c: In function ‘shmem_link’:
   test.c:60:9: warning: ‘ret’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
     return ret;
            ^~~

*does* show the expected warning.

So it is the presence of that

      if (ret) return ret;

that suppresses the warning.

What I *suspect* happens is

 (a) gcc sees that there is only one assignment to "ret"

 (b) in the same basic block as the assignment, there is a test
against "ret" being nonzero that goes out.

and what I think happens is that (a) causes gcc to consider that
assignment to be the defining assignment (which makes all kinds of
sense in an SSA world), and then (b) means that gcc decides that
clearly "ret" has to be zero in any case that doesn't go out due to
the if-test.

In fact, if I then look at the code generation, gcc will actually do
this (edited to be more legible):

        movl    (%rbx), %eax       <- load inode->i_nlink
        testl   %eax, %eax
        je      .L1
       ...
       ...
        call    d_instantiate
        xorl    %eax, %eax     <- explicitly zero 'ret'!
.L1:
        popq    %rbx
        popq    %rbp
        popq    %r12
        ret

so at least with my compiler, it *effectively* zeroed ret (in %rax)
anyway, and it all just _happened_ to get the right result even though
'ret' wasn't actually initialized.

Which is why it all worked just fine. And depending on how gcc works
internally, it really may not just be a random mistake of register
allocation, but really because gcc kind of _thought_ that 'ret' was
zero-initialized due to the combination of the one single assigment
and test for zero.

So it turns out that the patch to initialize to zero doesn't do
anything, probably for the same reason that gcc didn't warn about the
missing initialization. Gcc kind of added an initialization of its own
there.

I'm not entirely sure if any gcc developer would be interested in this
as a test-case, but I guess I can try to do a bugzilla.

Adding a few gcc people who have been on previous kernel gcc bugzilla
discussions, just in case they have something to add.

The gcc bugzilla is this:

    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89501

and I tried to make it be self-explanatory, but I wrote the bugzilla
in parallel with this email, and maybe there's some missing context
either there (or here).

                 Linus

[-- Attachment #2: test.c --]
[-- Type: text/x-c-code, Size: 1531 bytes --]

/*
 * Minimal fake declarations of "kernel" data types
 */
struct superblock;

struct inode {
	int i_nlink;
	int i_size;
	int i_ctime;
	int i_mtime;
	struct superblock *i_sb;
};

struct dentry {
	struct inode *d_inode;
};

#define d_inode(dentry) ((dentry)->d_inode)

extern int current_time(struct inode *);
extern void inc_nlink(struct inode *);
extern void ihold(struct inode *);
extern void dget(struct dentry *);
extern void d_instantiate(struct dentry *, struct inode *);
extern int shmem_reserve_inode(struct superblock *);

#define BOGO_DIRENT_SIZE 20

/*
 * The actual function where I'd have expected a warning
 * about "ret might be used uninitialized"
 */
int shmem_link(struct dentry *old_dentry, struct inode *dir,
		      struct dentry *dentry)
{
	struct inode *inode = d_inode(old_dentry);
	int ret;

	/*
	 * No ordinary (disk based) filesystem counts links as inodes;
	 * but each new link needs a new dentry, pinning lowmem, and
	 * tmpfs dentries cannot be pruned until they are unlinked.
	 * But if an O_TMPFILE file is linked into the tmpfs, the
	 * first link must skip that, to get the accounting right.
	 */
	if (inode->i_nlink) {
		ret = shmem_reserve_inode(inode->i_sb);
#ifndef HIDE_PROBLEM
		if (ret)
			return ret;
#endif
	}

	dir->i_size += BOGO_DIRENT_SIZE;
	inode->i_ctime = dir->i_ctime = dir->i_mtime = current_time(inode);
	inc_nlink(inode);
	ihold(inode);		/* New dentry reference */
	dget(dentry);		/* Extra pinning count for the created dentry */
	d_instantiate(dentry, inode);
	return ret;
}