| 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
| ||
|
Message-ID: <20240827092433.xzyslhyqtlsnpzz3@quack3>
Date: Tue, 27 Aug 2024 11:24:33 +0200
From: Jan Kara <jack@...e.cz>
To: Mirsad Todorovac <mtodorovac69@...il.com>
Cc: Jan Kara <jack@...e.cz>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>,
Kees Cook <kees@...nel.org>, Christian Brauner <brauner@...nel.org>,
"Matthew Wilcox (Oracle)" <willy@...radead.org>,
Al Viro <viro@...iv.linux.org.uk>, Jeff Layton <jlayton@...nel.org>,
reiserfs-devel@...r.kernel.org
Subject: Re: [PROBLEM linux-next] fs/reiserfs/do_balan.c:1147:13: error: variable ‘leaf_mi’ set but not used [-Werror=unused-but-set-variable]
On Sun 11-08-24 22:52:03, Mirsad Todorovac wrote:
> On 8/11/24 15:34, Mirsad Todorovac wrote:
> > On 8/6/24 10:25, Jan Kara wrote:
> >> On Mon 05-08-24 23:24:06, Mirsad Todorovac wrote:
> >>> On 8/5/24 15:04, Jan Kara wrote:
> >>>> On Fri 02-08-24 18:31:46, Mirsad Todorovac wrote:
> >>>>> On 7/18/24 11:39, Jan Kara wrote:
> >>>>>> On Thu 18-07-24 00:14:24, Mirsad Todorovac wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 7/17/24 17:44, Jan Kara wrote:
> >>>>>>>> On Tue 16-07-24 19:17:05, Mirsad Todorovac wrote:
> >>>>>>>>> On 7/15/24 19:28, Jan Kara wrote:
> >>>>>>>>>> Hello Mirsad!
> >>>>>>>>>>
> >>>>>>>>>> On Wed 10-07-24 20:09:27, Mirsad Todorovac wrote:
> >>>>>>>>>>> On the linux-next vanilla next-20240709 tree, I have attempted the seed KCONFIG_SEED=0xEE7AB52F
> >>>>>>>>>>> which was known from before to trigger various errors in compile and build process.
> >>>>>>>>>>>
> >>>>>>>>>>> Though this might seem as contributing to channel noise, Linux refuses to build this config,
> >>>>>>>>>>> treating warnings as errors, using this build line:
> >>>>>>>>>>>
> >>>>>>>>>>> $ time nice make W=1 -k -j 36 |& tee ../err-next-20230709-01a.log; date
> >>>>>>>>>>>
> >>>>>>>>>>> As I know that the Chief Penguin doesn't like warnings, but I am also aware that there are plenty
> >>>>>>>>>>> left, there seems to be more tedious work ahead to make the compilers happy.
> >>>>>>>>>>>
> >>>>>>>>>>> The compiler output is:
> >>>>>>>>>>>
> >>>>>>>>>>> ---------------------------------------------------------------------------------------------------------
> >>>>>>>>>>> fs/reiserfs/do_balan.c: In function ‘balance_leaf_new_nodes_paste_whole’:
> >>>>>>>>>>> fs/reiserfs/do_balan.c:1147:13: error: variable ‘leaf_mi’ set but not used [-Werror=unused-but-set-variable]
> >>>>>>>>>>> 1147 | int leaf_mi;
> >>>>>>>>>>> | ^~~~~~~
> >>>>>>>>>>
> >>>>>>>>>> Frankly, I wouldn't bother with reiserfs. The warning is there for ages,
> >>>>>>>>>> the code is going to get removed in two releases, so I guess we can live
> >>>>>>>>>> with these warnings for a few more months...
> >>>>>>>>>
> >>>>>>>>> In essence I agree with you, but for sentimental reasons I would like to
> >>>>>>>>> keep it because it is my first journaling Linux system on Knoppix 🙂
> >>>>>>>>
> >>>>>>>> As much as I understand your sentiment (I have a bit of history with that
> >>>>>>>> fs as well) the maintenance cost isn't really worth it and most fs folks
> >>>>>>>> will celebrate when it's removed. We have already announced the removal
> >>>>>>>> year and half ago and I'm fully for executing that plan at the end of this
> >>>>>>>> year.
> >>>>>>>>
> >>>>>>>>> Patch is also simple and a no-brainer, as proposed by Mr. Cook:
> >>>>>>>>>
> >>>>>>>>> -------------------------------><------------------------------------------
> >>>>>>>>> diff --git a/fs/reiserfs/do_balan.c b/fs/reiserfs/do_balan.c
> >>>>>>>>> index 5129efc6f2e6..fbe73f267853 100644
> >>>>>>>>> --- a/fs/reiserfs/do_balan.c
> >>>>>>>>> +++ b/fs/reiserfs/do_balan.c
> >>>>>>>>> @@ -1144,7 +1144,9 @@ static void balance_leaf_new_nodes_paste_whole(struct tree_balance *tb,
> >>>>>>>>> {
> >>>>>>>>> struct buffer_head *tbS0 = PATH_PLAST_BUFFER(tb->tb_path);
> >>>>>>>>> int n = B_NR_ITEMS(tbS0);
> >>>>>>>>> +#ifdef CONFIG_REISERFS_CHECK
> >>>>>>>>> int leaf_mi;
> >>>>>>>>> +#endif
> >>>>>>>>
> >>>>>>>> Well, I would not like this even for actively maintained code ;) If you
> >>>>>>>> want to silence these warnings in this dead code, then I could live with
> >>>>>>>> something like:
> >>>>>>>>
> >>>>>>>> #if defined( CONFIG_REISERFS_CHECK )
> >>>>>>>> #define RFALSE(cond, format, args...) __RASSERT(!(cond), ....)
> >>>>>>>> #else
> >>>>>>>> - #define RFALSE( cond, format, args... ) do {;} while( 0 )
> >>>>>>>> + #define RFALSE( cond, format, args... ) do { (void)cond; } while( 0 )
> >>>>>>>> #endif
> >>>>>>>
> >>>>>>> Yes, one line change is much smarter than 107 line patch of mine :-)
> >>>>>>>
> >>>>>>> Verified, and this line solved all the warnings:
> >>>>>>>
> >>>>>>> CC fs/reiserfs/bitmap.o
> >>>>>>> CC fs/reiserfs/do_balan.o
> >>>>>>> CC fs/reiserfs/namei.o
> >>>>>>> CC fs/reiserfs/inode.o
> >>>>>>> CC fs/reiserfs/file.o
> >>>>>>> CC fs/reiserfs/dir.o
> >>>>>>> CC fs/reiserfs/fix_node.o
> >>>>>>> CC fs/reiserfs/super.o
> >>>>>>> CC fs/reiserfs/prints.o
> >>>>>>> CC fs/reiserfs/objectid.o
> >>>>>>> CC fs/reiserfs/lbalance.o
> >>>>>>> CC fs/reiserfs/ibalance.o
> >>>>>>> CC fs/reiserfs/stree.o
> >>>>>>> CC fs/reiserfs/hashes.o
> >>>>>>> CC fs/reiserfs/tail_conversion.o
> >>>>>>> CC fs/reiserfs/journal.o
> >>>>>>> CC fs/reiserfs/resize.o
> >>>>>>> CC fs/reiserfs/item_ops.o
> >>>>>>> CC fs/reiserfs/ioctl.o
> >>>>>>> CC fs/reiserfs/xattr.o
> >>>>>>> CC fs/reiserfs/lock.o
> >>>>>>> CC fs/reiserfs/procfs.o
> >>>>>>> AR fs/reiserfs/built-in.a
> >>>>>>>
> >>>>>>> Just FWIW, back then in year 2000/2001 a journaling file system on my
> >>>>>>> Knoppix box was a quantum leap - it would simply replay the journal if
> >>>>>>> there was a power loss before shutdown. No several minutes of fsck.
> >>>>>>
> >>>>>> Well, there was also ext3 at that time already :-) That's where I became
> >>>>>> familiar with the idea of journalling. Reiserfs was interesting to me
> >>>>>> because of completely different approach to on-disk format (b-tree with
> >>>>>> formatted items), packing of small files / file tails (interesting in 2000,
> >>>>>> not so much 20 years later) and reasonable scalability for large
> >>>>>> directories.
> >>>>>>
> >>>>>>> I think your idea is great and if you wish a patch could be submitted
> >>>>>>> after the merge window.
> >>>>>>
> >>>>>> I'll leave it up to you. If the warnings annoy you, send the patch along
> >>>>>> the lines I've proposed (BTW (void)cond should better be (void)(cond) for
> >>>>>> macro safety) and I'll push it to Linus.
> >>>>>>
> >>>>>> Honza
> >>>>>
> >>>>> Hi, Jan,
> >>>>>
> >>>>> After a short break, I just tried a full build with this hack against the vanilla
> >>>>> linux-next tree:
> >>>>>
> >>>>> #define RFALSE( cond, format, args... ) do { (void)(cond); } while( 0 )
> >>>>>
> >>>>> and it breaks at least here:
> >>>>>
> >>>>> In file included from fs/reiserfs/do_balan.c:15:
> >>>>> fs/reiserfs/do_balan.c: In function ‘balance_leaf_when_delete_del’:
> >>>>> fs/reiserfs/do_balan.c:86:28: error: ‘ih’ undeclared (first use in this function)
> >>>>> 86 | RFALSE(ih_item_len(ih) + IH_SIZE != -tb->insert_size[0],
> >>>>> | ^~
> >>>>> fs/reiserfs/reiserfs.h:919:54: note: in definition of macro ‘RFALSE’
> >>>>> 919 | #define RFALSE( cond, format, args... ) do { (void) (cond); } while( 0 )
> >>>>> | ^~~~
> >>>>> ./include/linux/byteorder/generic.h:91:21: note: in expansion of macro ‘__le16_to_cpu’
> >>>>> 91 | #define le16_to_cpu __le16_to_cpu
> >>>>> | ^~~~~~~~~~~~~
> >>>>> fs/reiserfs/do_balan.c:86:16: note: in expansion of macro ‘ih_item_len’
> >>>>> 86 | RFALSE(ih_item_len(ih) + IH_SIZE != -tb->insert_size[0],
> >>>>> | ^~~~~~~~~~~
> >>>>> fs/reiserfs/do_balan.c:86:28: note: each undeclared identifier is reported only once for each function it appears in
> >>>>> 86 | RFALSE(ih_item_len(ih) + IH_SIZE != -tb->insert_size[0],
> >>>>> | ^~
> >>>>> fs/reiserfs/reiserfs.h:919:54: note: in definition of macro ‘RFALSE’
> >>>>> 919 | #define RFALSE( cond, format, args... ) do { (void) (cond); } while( 0 )
> >>>>> | ^~~~
> >>>>> ./include/linux/byteorder/generic.h:91:21: note: in expansion of macro ‘__le16_to_cpu’
> >>>>> 91 | #define le16_to_cpu __le16_to_cpu
> >>>>> | ^~~~~~~~~~~~~
> >>>>> fs/reiserfs/do_balan.c:86:16: note: in expansion of macro ‘ih_item_len’
> >>>>> 86 | RFALSE(ih_item_len(ih) + IH_SIZE != -tb->insert_size[0],
> >>>>> | ^~~~~~~~~~~
> >>>>> fs/reiserfs/do_balan.c: In function ‘do_balance_starts’:
> >>>>> fs/reiserfs/do_balan.c:1800:16: error: implicit declaration of function ‘check_before_balancing’ [-Werror=implicit-function-declaration]
> >>>>> 1800 | RFALSE(check_before_balancing(tb), "PAP-12340: locked buffers in TB");
> >>>>> | ^~~~~~~~~~~~~~~~~~~~~~
> >>>>> fs/reiserfs/reiserfs.h:919:54: note: in definition of macro ‘RFALSE’
> >>>>> 919 | #define RFALSE( cond, format, args... ) do { (void) (cond); } while( 0 )
> >>>>> | ^~~~
> >>>>> cc1: some warnings being treated as errors
> >>>>> make[7]: *** [scripts/Makefile.build:244: fs/reiserfs/do_balan.o] Error 1
> >>>>> CC [M] fs/reiserfs/stree.o
> >>>>> In file included from fs/reiserfs/stree.c:15:
> >>>>> fs/reiserfs/stree.c: In function ‘reiserfs_delete_item’:
> >>>>> fs/reiserfs/stree.c:1283:24: error: ‘mode’ undeclared (first use in this function)
> >>>>> 1283 | RFALSE(mode != M_DELETE, "PAP-5320: mode must be M_DELETE");
> >>>>> | ^~~~
> >>>>> fs/reiserfs/reiserfs.h:919:54: note: in definition of macro ‘RFALSE’
> >>>>> 919 | #define RFALSE( cond, format, args... ) do { (void) (cond); } while( 0 )
> >>>>> | ^~~~
> >>>>> fs/reiserfs/stree.c:1283:24: note: each undeclared identifier is reported only once for each function it appears in
> >>>>> 1283 | RFALSE(mode != M_DELETE, "PAP-5320: mode must be M_DELETE");
> >>>>> | ^~~~
> >>>>> fs/reiserfs/reiserfs.h:919:54: note: in definition of macro ‘RFALSE’
> >>>>> 919 | #define RFALSE( cond, format, args... ) do { (void) (cond); } while( 0 )
> >>>>> | ^~~~
> >>>>>
> >>>>> Last time it compiled, but now it expects variables in (void)(cond) expressions to be defined.
> >>>>>
> >>>>> I have try to fix those warnings, submitting the patch for review:
> >>>>
> >>>> Yeah, this looks ok to me.
> >>>>
> >>>> Honza
> >>>
> >>> Hi, Jan,
> >>>
> >>> As I understood from the previous experiences, and the Code of Conduct,
> >>> and explicit Reviwed-by: or Acked-by: is required ...
> >>>
> >>> Or otherwise, the Suggested-by: is used?
> >>
> >> So I was waiting for you to submit official patch with proper changelog and
> >> your Signed-off-by. Then I can pick up the patch into my tree and merge it.
> >>
> >> Honza
> >
> > Aaah, sorry I've just noticed your reply. I missed it this morning already.
> >
> > Yes, at your request, I will proceed as you recommended.
>
> Hi, Jan,
>
> As the filesystem has problems with the "General Protection Fault", which I learned later and
> I am really not qualified to deal with that, just fixing compiler warnings is indeed cosmetics and an
> exercise for the little grey cells.
>
> Es ist nicht meine ernst, as Germans would have said.
>
> But I will trust your judgment on whether this is worth patching.
As I wrote earlier, I don't think fixing warning is reiserfs code is really
worth it. It is like polishing windows on a car you're going to send to
scrapyard because the engine is broken :). But Linux is a shared project so
I also don't want to block other people from doing things they find
sensible... That's why if you send me a proper patch for this, I'll just
merge it.
Honza
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists