linux-kernel - Re: [PATCH] btrfs: add fs state details to error messages.

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: <dc7bfe199bf22e67061a358e8e1f32b2@dorminy.me>
Date:   Tue, 15 Feb 2022 11:07:01 -0500
From:   Sweet Tea Dorminy <sweettea-kernel@...miny.me>
To:     Sweet Tea Dorminy <sweettea-kernel@...miny.me>
Cc:     Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs: add fs state details to error messages.


>> +static void btrfs_state_to_string(const struct btrfs_fs_info *info, 
>> char *buf)
>> +{
>> +	unsigned long state = info->fs_state;
>> +	char *curr = buf;
>> +
>> +	memcpy(curr, STATE_STRING_PREFACE, sizeof(STATE_STRING_PREFACE));
>> +	curr += sizeof(STATE_STRING_PREFACE) - 1;
>> +
>> +	/* If more states are reported, update MAX_STATE_CHARS also */
>> +	if (test_and_clear_bit(BTRFS_FS_STATE_ERROR, &state))
> 
> The state is supposed to be sticky, so can't be cleared. Also as I read
> the suggested change, the "state: " should be visible for all messages
> that are printed after the filesystem state changes.

'state' is a local copy of info->fs_state, so clearing bits on the local 
copy should be okay, and the "state: " will be present for everything 
after the fs state changes. Could instead use test_bit(&info->fs_state) 
and keep a count of how many states were printed (to know if we need to 
reset the buffer) if that is clearer.
> 
>> +		*curr++ = 'E';
>> +
>> +	if (test_and_clear_bit(BTRFS_FS_STATE_TRANS_ABORTED, &state))
>> +		*curr++ = 'X';
>> +
>> +	/* If no states were printed, reset the buffer */
>> +	if (state == info->fs_state)
>> +		curr = buf;
>> +
>> +	*curr++ = '\0';
>> +}
>> +
>>  /*
>>   * Generally the error codes correspond to their respective errors, 
>> but there
>>   * are a few special cases.
>> @@ -128,6 +153,7 @@ void __btrfs_handle_fs_error(struct btrfs_fs_info 
>> *fs_info, const char *function
>>  {
>>  	struct super_block *sb = fs_info->sb;
>>  #ifdef CONFIG_PRINTK
>> +	char statestr[sizeof(STATE_STRING_PREFACE) + MAX_STATE_CHARS];
>>  	const char *errstr;
>>  #endif
>> 
>> @@ -136,10 +162,11 @@ void __btrfs_handle_fs_error(struct 
>> btrfs_fs_info *fs_info, const char *function
>>  	 * under SB_RDONLY, then it is safe here.
>>  	 */
>>  	if (errno == -EROFS && sb_rdonly(sb))
>> -  		return;
>> +		return;
> 
> Unnecessary change.
Yeah, there's a stray space at the start of that line, but I can take 
out fixing it.
> 
>> 
>>  #ifdef CONFIG_PRINTK
>>  	errstr = btrfs_decode_error(errno);
>> +	btrfs_state_to_string(fs_info, statestr);
>>  	if (fmt) {
>>  		struct va_format vaf;
>>  		va_list args;
>> @@ -148,12 +175,12 @@ void __btrfs_handle_fs_error(struct 
>> btrfs_fs_info *fs_info, const char *function
>>  		vaf.fmt = fmt;
>>  		vaf.va = &args;
>> 
>> -		pr_crit("BTRFS: error (device %s) in %s:%d: errno=%d %s (%pV)\n",
>> -			sb->s_id, function, line, errno, errstr, &vaf);
>> +		pr_crit("BTRFS: error (device %s%s) in %s:%d: errno=%d %s (%pV)\n",
>> +			sb->s_id, statestr, function, line, errno, errstr, &vaf);
> 
> Alternatively the state message can be built into the message itself so
> it does not require the extra array.
> 
> 
> 		pr_crit("btrfs: error(device %s%s%s%s) ...",
> 			sb->s_id,
> 			statebits ? ", state" : "",
> 			test_bit(FS_ERRROR) ? "E" : "",
> 			test_bit(TRANS_ABORT) ? "T" : "", ...);
> 
> This is the idea, final code can use some wrappers around the bit
> constatnts.
> 
> 
>>  		va_end(args);
>>  	} else {
>> -		pr_crit("BTRFS: error (device %s) in %s:%d: errno=%d %s\n",
>> -			sb->s_id, function, line, errno, errstr);
>> +		pr_crit("BTRFS: error (device %s%s) in %s:%d: errno=%d %s\n",
>> +			sb->s_id, statestr, function, line, errno, errstr);
> 
> Filling the temporary array makes sense as it's used twice, however I'm
> not sure if the 'else' branch is ever executed.
There are a bunch of calls with NULL format -> else branch, 
unfortunately.

Thanks!