tmpfs: fix uninitialized return value in shmem_link

From: Darrick J. Wong <[email protected]>

When we made the shmem_reserve_inode call in shmem_link conditional, we forgot to update the declaration for ret so that it always has a known value. Dan Carpenter pointed out this deficiency in the original patch.

---
 mm/shmem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 0905215fb016..2c012eee133d 100644
@@ -2848,7 +2848,7 @@ static int shmem_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 static int shmem_link(struct dentry *old_dentry, struct inode *dir, struct dentry *dentry)
 {
 	struct inode *inode = d_inode(old_dentry);
-	int ret;
+	int ret = 0;
 
 	/*
 	 * No ordinary (disk based) filesystem counts links as inodes;

Gosh, yes indeed: thanks Dan and Darrick, I'm very sorry for not noticing that.

(and sorry if this mail is garbled: it's from gmail, I cannot use alpine at the moment).

From: "Darrick J. Wong" <[email protected]>

When we made the shmem_reserve_inode call in shmem_link conditional, we forgot to update the declaration for ret so that it always has a known value. Dan Carpenter pointed out this deficiency in the original patch.

---
 mm/shmem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 0905215fb016..2c012eee133d 100644
@@ -2848,7 +2848,7 @@ static int shmem_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 static int shmem_link(struct dentry *old_dentry, struct inode *dir, struct dentry *dentry)
 {
 	struct inode *inode = d_inode(old_dentry);
-	int ret;
+	int ret = 0;
 
 	/*
 	 * No ordinary (disk based) filesystem counts links as inodes;

Applied.

Side note: how come gcc didn't warn about this? Yes, we disable that warning for some cases because of lots of false positives, but I thought the *default* setup still had it.

Is it just that the goto ends up confusing gcc enough that it never notices?

Linus

Thanks. And I apologize for letting that slip through: Darrick sent the patch fragment, I dressed it up, and more or less tricked him into taking ownership of the bug, when it's I who should have been more careful.

But I'm glad it confirmed your rc8 instinct, rather than messing final :)

I thought so too, and have been puzzled by it. If I try removing the initialization of inode from the next function, shmem_unlink(), I do get the expected warning for that.

Since the goto route did have ret properly initialized, I don't see why it might have been confusing, but what do I know...

I thought it might be because outside the goto route, ret was used for nothing but the return value. But that's disproved: I tried a very silly "inode->i_flags = ret;" just after d_instantiate(), and still no warning when ret is uninitialized.

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.

Hugh

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

/*
* 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;
}

Well, that was quick. Yup, it's considered a gcc bug.

Sadly, it's just a different version of a really old bug:

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

which goes back to 2004.

Which I guess means we should not expect this to be fixed in gcc any time soon.

The *good* news (I guess) is that if we have other situations with that pattern, and that lack of warning, it really is because gcc will have generated code as if it was initialized (to the value that we tested it must have been in the one basic block where it *was* initialized).

So it won't leak random kernel data, and with the common error condition case (like in this example - checking that we didn't have an error) it will actually end up doing the right thing.

Entirely by mistake, and without a warniing, but still.. It could have been much worse. Basically at least for this pattern, "lack of warning" ends up meaning "it got initialized to the expected value".

Of course, that's just gcc. I have no idea what llvm ends up doing.

Linus

Clang 7.0:

# clang  -O2 -S -Wall /tmp/test.c
/tmp/test.c:46:6: warning: variable 'ret' is used uninitialized whenever 'if'
condition is false [-Wsometimes-uninitialized]
        if (inode->i_nlink) {
            ^~~~~~~~~~~~~~
/tmp/test.c:60:9: note: uninitialized use occurs here
        return ret;
               ^~~
/tmp/test.c:46:2: note: remove the 'if' if its condition is always true
        if (inode->i_nlink) {
        ^~~~~~~~~~~~~~~~~~~~
/tmp/test.c:37:9: note: initialize the variable 'ret' to silence this warning
        int ret;
               ^
                = 0
1 warning generated.

Ok, good.

Do we have any clang builds in any of the zero-day robot infrastructure or something? Should we?

And maybe this was how Dan noticed the problem in the first place? Or is it just because of his eagle-eyes?

Linus

He didn't say specifically how he found it, but I would guess he was running smatch...?

--D

BTW, even clang is able to generate warnings in your sample code, it does not generate any warnings when compiling the buggy shmem.o via "make CC=clang". Here is the objdump for arm64 (with KASAN_SW_TAGS inline).

000000000000effc <shmem_link>:
{
    effc:       f81c0ff7        str     x23, [sp, #-64]!
    f000:       a90157f6        stp     x22, x21, [sp, #16] f004:       a9024ff4        stp     x20, x19, [sp, #32] f008:       a9037bfd        stp     x29, x30, [sp, #48] f00c:       9100c3fd        add     x29, sp, #0x30
    f010:       aa0203f3        mov     x19, x2
    f014:       aa0103f5        mov     x21, x1
    f018:       aa0003f4        mov     x20, x0
    f01c:       94000000        bl      0 <_mcount>
    f020:       91016280        add     x0, x20, #0x58
    f024:       d2c20017        mov     x23, #0x100000000000            // #17592186044416
    f028:       b2481c08        orr     x8, x0, #0xff00000000000000 f02c:       f2fdfff7        movk    x23, #0xefff, lsl #48 f030:       d344fd08        lsr     x8, x8, #4
    f034:       38776909        ldrb    w9, [x8, x23]
    f038:       940017d5        bl      14f8c <OUTLINED_FUNCTION_11> f03c:       54000060        b.eq    f048 <shmem_link+0x4c>  // b.none f040:       7103fd1f        cmp     w8, #0xff
    f044:       54000981        b.ne    f174 <shmem_link+0x178>  // b.any f048:       f9400014        ldr     x20, [x0]
        if (inode->i_nlink) {
    f04c:       91010280        add     x0, x20, #0x40
    f050:       b2481c08        orr     x8, x0, #0xff00000000000000 f054:       d344fd08        lsr     x8, x8, #4
    f058:       38776909        ldrb    w9, [x8, x23]
    f05c:       940017cc        bl      14f8c <OUTLINED_FUNCTION_11> f060:       54000060        b.eq    f06c <shmem_link+0x70>  // b.none f064:       7103fd1f        cmp     w8, #0xff
    f068:       540008a1        b.ne    f17c <shmem_link+0x180>  // b.any f06c:       b9400008        ldr     w8, [x0]
    f070:       34000148        cbz     w8, f098 <shmem_link+0x9c> f074:       940018fd        bl      15468 <OUTLINED_FUNCTION_1124> ret = shmem_reserve_inode(inode->i_sb);
    f078:       38776909        ldrb    w9, [x8, x23]
    f07c:       940017c4        bl      14f8c <OUTLINED_FUNCTION_11> f080:       54000060        b.eq    f08c <shmem_link+0x90>  // b.none f084:       7103fd1f        cmp     w8, #0xff
    f088:       540007e1        b.ne    f184 <shmem_link+0x188>  // b.any f08c:       f9400000        ldr     x0, [x0]
    f090:       97fffcf6        bl      e468 <shmem_reserve_inode> if (ret)
    f094:       35000660        cbnz    w0, f160 <shmem_link+0x164> dir->i_size += BOGO_DIRENT_SIZE;
    f098:       910122a0        add     x0, x21, #0x48
    f09c:       b2481c08        orr     x8, x0, #0xff00000000000000 f0a0:       d344fd09        lsr     x9, x8, #4
    f0a4:       3877692a        ldrb    w10, [x9, x23]
    f0a8:       94001828        bl      15148 <OUTLINED_FUNCTION_193> f0ac:       54000060        b.eq    f0b8 <shmem_link+0xbc>  // b.none f0b0:       7103fd1f        cmp     w8, #0xff
    f0b4:       540006c1        b.ne    f18c <shmem_link+0x190>  // b.any f0b8:       38776929        ldrb    w9, [x9, x23]
    f0bc:       94001a4a        bl      159e4 <OUTLINED_FUNCTION_1131> f0c0:       54000060        b.eq    f0cc <shmem_link+0xd0>  // b.none f0c4:       7103fd1f        cmp     w8, #0xff
    f0c8:       54000661        b.ne    f194 <shmem_link+0x198>  // b.any f0cc:       f9000009        str     x9, [x0]
        inode->i_ctime = dir->i_ctime = dir->i_mtime = current_time(inode); f0d0:       aa1403e0        mov     x0, x20
    f0d4:       910182b6        add     x22, x21, #0x60
    f0d8:       94000000        bl      0 <current_time>
    f0dc:       b2481ec9        orr     x9, x22, #0xff00000000000000 f0e0:       d344fd29        lsr     x9, x9, #4

Ah, thanks to the commit 6e8d666e9253 ("Disable "maybe-uninitialized" warning globally"), it will no longer generate this type of warnings until using "make W=1" due to the commit a76bcf557ef4 ("Kbuild: enable -Wmaybe-uninitialized warning for 'make W=1'"). Anyway, the generated code is the same using clang with and without this patch.

    d_instantiate(dentry, inode);

4eec:       94000000        bl      0 <d_instantiate> ret = shmem_reserve_inode(inode->i_sb);
4ef0:       2a1f03e0        mov     w0, wzr <---- ret = 0
    return ret;

Unfortunately, scripts/Kbuild.extrawarn disables -Wuninitialized for Clang, which also disables -Wsometimes-uninitialized:

https://github.com/ClangBuiltLinux/linux/issues/381
https://clang.llvm.org/docs/DiagnosticsReference.html#wuninitialized

I'm going to be sending out patches to fix the warnings found with it then enable it going forward so that things like this get caught.

Nathan