linux-ext4 - Re: [PATCH] commit 1f981816ac8d855cc2af6150711f3417e02e12bf Author: Andreas Dilger <andreas.dilger@...el.com> Date: Fri Apr 13 02:01:12 2012 -0600

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] [day] [month] [year] [list]
Message-ID: <20180702180534.GD30481@thunk.org>
Date:   Mon, 2 Jul 2018 14:05:34 -0400
From:   "Theodore Y. Ts'o" <tytso@....edu>
To:     c17828 <artem.blagodarenko@...il.com>
Cc:     linux-ext4@...r.kernel.org, adilger.kernel@...ger.ca,
        alexey.lyashkov@...il.com,
        Andreas Dilger <andreas.dilger@...el.com>
Subject: Re: [PATCH] commit 1f981816ac8d855cc2af6150711f3417e02e12bf Author:
 Andreas Dilger <andreas.dilger@...el.com> Date:   Fri Apr 13 02:01:12 2012
 -0600

On Mon, Jul 02, 2018 at 07:14:16PM +0300, c17828 wrote:
> tests: attr checking code test
> 
> Add a regression test for in-inode xattrs.
> 
> This is part of "e2fsck: clean up xattr checking code, add test" patch.
> Cleanup is removed becaus is not actual for this codebase.

This commit description doesn't really explain what the test does.

The test is also a complete no-op.  Alas, it appears it was never
seriously tested, at least not recently.  (See below for more
details.)

> --- /dev/nullt
> +++ b/lib/ext2fs/tst_read_ea.c
> @@ -0,0 +1,239 @@
> +/*
> + * tst_getsize.c --- this function tests the getsize function

The header comment was never adjusted after test was cribbed from
tst_getsize.c.

> + * %Begin-Header%
> + * This file may be redistributed under the terms of the GNU Public
> + * License.
> + * %End-Header%
> + */
> +
> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE	/* for asprintf */
> +#endif
> +#include <stdio.h>

Before all of the header declarations, you have to include "config.h".
Otherwise, all of the HAVE_UNISTD_H, or more critically, HAVE_MNTENT_H
was never defined:

> +int main(int argc, const char *argv[])
> +{
> +#if HAVE_MNTENT_H
> +	ext2_filsys fs;

As a result, the tst_read_ea program is one gigantic no-op:

% size tst_read_ea.o
   text	   data	    bss	    dec	    hex	filename
     51	      0	      0	     51	     33	tst_read_ea.o

I tried adding the missing '#include "config.h' line, at which point
tst_read_ea.c fails to compile:

/usr/projects/e2fsprogs/e2fsprogs-maint/lib/ext2fs/tst_read_ea.c: In function ‘get_xattrs2’:
/usr/projects/e2fsprogs/e2fsprogs-maint/lib/ext2fs/tst_read_ea.c:150:9: warning: implicit declaration of function ‘ext2fs_attr_get’; did you mean ‘ext2fs_xattr_get’? [-Wimplicit-function-declaration]
   err = ext2fs_attr_get(fs, inode, EXT2_ATTR_INDEX_USER,
         ^~~~~~~~~~~~~~~
         ext2fs_xattr_get
/usr/projects/e2fsprogs/e2fsprogs-maint/lib/ext2fs/tst_read_ea.c:150:36: error: ‘EXT2_ATTR_INDEX_USER’ undeclared (first use in this function); did you mean ‘EXT_LAST_INDEX’?
   err = ext2fs_attr_get(fs, inode, EXT2_ATTR_INDEX_USER,
                                    ^~~~~~~~~~~~~~~~~~~~
                                    EXT_LAST_INDEX
/usr/projects/e2fsprogs/e2fsprogs-maint/lib/ext2fs/tst_read_ea.c:150:36: note: each undeclared identifier is reported only once for each function it appears in
make: *** [Makefile:665: tst_read_ea.o] Error 1

It looks like tst_read_ea is using some older version of the xattr
support functions, and was never cleaned up when those functions went
upstream.  Apparently no one noticed because all of these calls were
compiled out.


More generally, this test depnds on user running this test to (a) be
able to find a currently mounted ext4 file system, (b) be able to open
its underlying block device, and (c) be able to write to the root
directory of the mounted file system.  In practice, it's going to
require root access.

A much better way of testing this would be to use a pre-made file
system, much all of the regression tests in the tests/ directory.  You
can either then use a stand-alone C program, or you can just use
debugfs.  (For example, see attached below.)

Anyway, I'm not going to apply this commit in its current state, and
I'd suggest completely working it in the Lustre's version of
e2fsprogs, since it's really not doing any good there.

Regards,

						- Ted

Script started on 2018-07-02 14:00:39-04:00

% /build/e2fsprogs-maint/debugfs/debugfs /tmp/foo.img
debugfs 1.44.3-rc1 (24-June-2018)
debugfs:  stat short
Inode: 13   Type: regular    Mode:  0644   Flags: 0x80000
Generation: 1647185658    Version: 0x00000000:00000001
User:     0   Group:     0   Project:     0   Size: 0
File ACL: 0
Links: 1   Blockcount: 0
Fragment:  Address: 0    Number: 0    Size: 0
 ctime: 0x5b3a671f:e8ffa0fc -- Mon Jul  2 13:55:43 2018
 atime: 0x5b3a670a:19faf30c -- Mon Jul  2 13:55:22 2018
 mtime: 0x5b3a670a:19faf30c -- Mon Jul  2 13:55:22 2018
crtime: 0x5b3a670a:19faf30c -- Mon Jul  2 13:55:22 2018
Size of extra inode fields: 32
Extended attributes:
  user.test (7) = "testing"
Inode checksum: 0xe53b328b
EXTENTS:
debugfs:  ea_get short user.test
user.test (7) = "testing"

debugfs:  stat long
Inode: 14   Type: regular    Mode:  0644   Flags: 0x80000
Generation: 2203030209    Version: 0x00000000:00000001
User:     0   Group:     0   Project:     0   Size: 0
File ACL: 1617
Links: 1   Blockcount: 2
Fragment:  Address: 0    Number: 0    Size: 0
 ctime: 0x5b3a6717:3656cf8c -- Mon Jul  2 13:55:35 2018
 atime: 0x5b3a6711:ab445038 -- Mon Jul  2 13:55:29 2018
 mtime: 0x5b3a6711:ab445038 -- Mon Jul  2 13:55:29 2018
crtime: 0x5b3a6711:ab445038 -- Mon Jul  2 13:55:29 2018
Size of extra inode fields: 32
Extended attributes:
  user.test (87)
Inode checksum: 0x00833190
EXTENTS:
debugfs:  ea_get long user.test
user.test (87) = "testing12345678901234567890123456789012345678901234567890123456789012345678901234567890"

debugfs:  stat inline
Inode: 12   Type: regular    Mode:  0644   Flags: 0x10000000
Generation: 1942101456    Version: 0x00000000:00000001
User:     0   Group:     0   Project:     0   Size: 88
File ACL: 0
Links: 1   Blockcount: 0
Fragment:  Address: 0    Number: 0    Size: 0
 ctime: 0x5b3a67bf:b7f94e2c -- Mon Jul  2 13:58:23 2018
 atime: 0x5b3a66fe:9af75700 -- Mon Jul  2 13:55:10 2018
 mtime: 0x5b3a67bf:b7f94e2c -- Mon Jul  2 13:58:23 2018
crtime: 0x5b3a66fe:9af75700 -- Mon Jul  2 13:55:10 2018
Size of extra inode fields: 32
Extended attributes:
  system.data (28)
Inode checksum: 0xedce581b
Size of inline data: 88
debugfs:  ea_get inline system.data
system.data (28) = "456789012345678901234567890\n"

debugfs:  inode_dump -b inline
0000  7465 7374 696e 6731 3233 3435 3637 3839  testing123456789
0020  3031 3233 3435 3637 3839 3031 3233 3435  0123456789012345
0040  3637 3839 3031 3233 3435 3637 3839 3031  6789012345678901
0060  3233 3435 3637 3839 3031 3233            234567890123

debugfs:  cat inline
testing12345678901234567890123456789012345678901234567890123456789012345678901234567890
debugfs:  inode_dump -x inline
magic = ea020000, length = 96, value_start =4 

offset = 4 (0004), name_len = 4, name_index = 7
value_offset = 64 (0104), value_inum = 0, value_size = 28
name = data
value = 456789012345678901234567890^J

last entry found at offset 24 (0030)
debugfs:  q
% exit

Script done on 2018-07-02 14:02:10-04:00