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  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]
Date:   Wed, 7 Oct 2020 10:11:08 +0200
From:   Jan Kara <jack@...e.cz>
To:     kernel test robot <lkp@...el.com>
Cc:     Jan Kara <jack@...e.cz>, Ted Tso <tytso@....edu>,
        kbuild-all@...ts.01.org, linux-ext4@...r.kernel.org,
        Fengnan Chang <changfengnan@...vision.com>,
        changfengnan <fengnanchang@...mail.com>
Subject: Re: [PATCH v3] jbd2: avoid transaction reuse after reformatting

On Tue 06-10-20 21:06:06, kernel test robot wrote:
> Hi Jan,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on ext4/dev]
> [also build test WARNING on linus/master v5.9-rc8 next-20201002]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]

Thanks for report! I'll send v4 with sparse warning fixed up (it isn't a
functional problem but it is good to have it fixed).

								Honza

> 
> url:    https://github.com/0day-ci/linux/commits/Jan-Kara/jbd2-avoid-transaction-reuse-after-reformatting/20201006-183337
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
> config: i386-randconfig-s001-20201005 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> reproduce:
>         # apt-get install sparse
>         # sparse version: v0.6.2-201-g24bdaac6-dirty
>         # https://github.com/0day-ci/linux/commit/ca606dc0beaec78e8b2b6ec2f112b2192534d9e2
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Jan-Kara/jbd2-avoid-transaction-reuse-after-reformatting/20201006-183337
>         git checkout ca606dc0beaec78e8b2b6ec2f112b2192534d9e2
>         # save the attached .config to linux build tree
>         make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@...el.com>
> 
> 	echo
> 	echo "sparse warnings: (new ones prefixed by >>)"
> 	echo
> >> fs/jbd2/recovery.c:713:41: sparse: sparse: incorrect type in initializer (different base types) @@     expected restricted __be64 [usertype] commit_time @@     got unsigned long long @@
>    fs/jbd2/recovery.c:713:41: sparse:     expected restricted __be64 [usertype] commit_time
> >> fs/jbd2/recovery.c:713:41: sparse:     got unsigned long long
>    fs/jbd2/recovery.c:730:45: sparse: sparse: restricted __be64 degrades to integer
>    fs/jbd2/recovery.c:730:60: sparse: sparse: restricted __be64 degrades to integer
> 
> vim +713 fs/jbd2/recovery.c
> 
>    415	
>    416	static int do_one_pass(journal_t *journal,
>    417				struct recovery_info *info, enum passtype pass)
>    418	{
>    419		unsigned int		first_commit_ID, next_commit_ID;
>    420		unsigned long		next_log_block;
>    421		int			err, success = 0;
>    422		journal_superblock_t *	sb;
>    423		journal_header_t *	tmp;
>    424		struct buffer_head *	bh;
>    425		unsigned int		sequence;
>    426		int			blocktype;
>    427		int			tag_bytes = journal_tag_bytes(journal);
>    428		__u32			crc32_sum = ~0; /* Transactional Checksums */
>    429		int			descr_csum_size = 0;
>    430		int			block_error = 0;
>    431		bool			need_check_commit_time = false;
>    432		__be64			last_trans_commit_time = 0;
>    433	
>    434		/*
>    435		 * First thing is to establish what we expect to find in the log
>    436		 * (in terms of transaction IDs), and where (in terms of log
>    437		 * block offsets): query the superblock.
>    438		 */
>    439	
>    440		sb = journal->j_superblock;
>    441		next_commit_ID = be32_to_cpu(sb->s_sequence);
>    442		next_log_block = be32_to_cpu(sb->s_start);
>    443	
>    444		first_commit_ID = next_commit_ID;
>    445		if (pass == PASS_SCAN)
>    446			info->start_transaction = first_commit_ID;
>    447	
>    448		jbd_debug(1, "Starting recovery pass %d\n", pass);
>    449	
>    450		/*
>    451		 * Now we walk through the log, transaction by transaction,
>    452		 * making sure that each transaction has a commit block in the
>    453		 * expected place.  Each complete transaction gets replayed back
>    454		 * into the main filesystem.
>    455		 */
>    456	
>    457		while (1) {
>    458			int			flags;
>    459			char *			tagp;
>    460			journal_block_tag_t *	tag;
>    461			struct buffer_head *	obh;
>    462			struct buffer_head *	nbh;
>    463	
>    464			cond_resched();
>    465	
>    466			/* If we already know where to stop the log traversal,
>    467			 * check right now that we haven't gone past the end of
>    468			 * the log. */
>    469	
>    470			if (pass != PASS_SCAN)
>    471				if (tid_geq(next_commit_ID, info->end_transaction))
>    472					break;
>    473	
>    474			jbd_debug(2, "Scanning for sequence ID %u at %lu/%lu\n",
>    475				  next_commit_ID, next_log_block, journal->j_last);
>    476	
>    477			/* Skip over each chunk of the transaction looking
>    478			 * either the next descriptor block or the final commit
>    479			 * record. */
>    480	
>    481			jbd_debug(3, "JBD2: checking block %ld\n", next_log_block);
>    482			err = jread(&bh, journal, next_log_block);
>    483			if (err)
>    484				goto failed;
>    485	
>    486			next_log_block++;
>    487			wrap(journal, next_log_block);
>    488	
>    489			/* What kind of buffer is it?
>    490			 *
>    491			 * If it is a descriptor block, check that it has the
>    492			 * expected sequence number.  Otherwise, we're all done
>    493			 * here. */
>    494	
>    495			tmp = (journal_header_t *)bh->b_data;
>    496	
>    497			if (tmp->h_magic != cpu_to_be32(JBD2_MAGIC_NUMBER)) {
>    498				brelse(bh);
>    499				break;
>    500			}
>    501	
>    502			blocktype = be32_to_cpu(tmp->h_blocktype);
>    503			sequence = be32_to_cpu(tmp->h_sequence);
>    504			jbd_debug(3, "Found magic %d, sequence %d\n",
>    505				  blocktype, sequence);
>    506	
>    507			if (sequence != next_commit_ID) {
>    508				brelse(bh);
>    509				break;
>    510			}
>    511	
>    512			/* OK, we have a valid descriptor block which matches
>    513			 * all of the sequence number checks.  What are we going
>    514			 * to do with it?  That depends on the pass... */
>    515	
>    516			switch(blocktype) {
>    517			case JBD2_DESCRIPTOR_BLOCK:
>    518				/* Verify checksum first */
>    519				if (jbd2_journal_has_csum_v2or3(journal))
>    520					descr_csum_size =
>    521						sizeof(struct jbd2_journal_block_tail);
>    522				if (descr_csum_size > 0 &&
>    523				    !jbd2_descriptor_block_csum_verify(journal,
>    524								       bh->b_data)) {
>    525					/*
>    526					 * PASS_SCAN can see stale blocks due to lazy
>    527	 				 * journal init. Don't error out on those yet.
>    528					 */
>    529					if (pass != PASS_SCAN) {
>    530						pr_err("JBD2: Invalid checksum "
>    531						       "recovering block %lu in log\n",
>    532						       next_log_block);
>    533						err = -EFSBADCRC;
>    534						brelse(bh);
>    535						goto failed;
>    536					}
>    537					need_check_commit_time = true;
>    538					jbd_debug(1,
>    539						"invalid descriptor block found in %lu\n",
>    540						next_log_block);
>    541				}
>    542	
>    543				/* If it is a valid descriptor block, replay it
>    544				 * in pass REPLAY; if journal_checksums enabled, then
>    545				 * calculate checksums in PASS_SCAN, otherwise,
>    546				 * just skip over the blocks it describes. */
>    547				if (pass != PASS_REPLAY) {
>    548					if (pass == PASS_SCAN &&
>    549					    jbd2_has_feature_checksum(journal) &&
>    550					    !need_check_commit_time &&
>    551					    !info->end_transaction) {
>    552						if (calc_chksums(journal, bh,
>    553								&next_log_block,
>    554								&crc32_sum)) {
>    555							put_bh(bh);
>    556							break;
>    557						}
>    558						put_bh(bh);
>    559						continue;
>    560					}
>    561					next_log_block += count_tags(journal, bh);
>    562					wrap(journal, next_log_block);
>    563					put_bh(bh);
>    564					continue;
>    565				}
>    566	
>    567				/* A descriptor block: we can now write all of
>    568				 * the data blocks.  Yay, useful work is finally
>    569				 * getting done here! */
>    570	
>    571				tagp = &bh->b_data[sizeof(journal_header_t)];
>    572				while ((tagp - bh->b_data + tag_bytes)
>    573				       <= journal->j_blocksize - descr_csum_size) {
>    574					unsigned long io_block;
>    575	
>    576					tag = (journal_block_tag_t *) tagp;
>    577					flags = be16_to_cpu(tag->t_flags);
>    578	
>    579					io_block = next_log_block++;
>    580					wrap(journal, next_log_block);
>    581					err = jread(&obh, journal, io_block);
>    582					if (err) {
>    583						/* Recover what we can, but
>    584						 * report failure at the end. */
>    585						success = err;
>    586						printk(KERN_ERR
>    587							"JBD2: IO error %d recovering "
>    588							"block %ld in log\n",
>    589							err, io_block);
>    590					} else {
>    591						unsigned long long blocknr;
>    592	
>    593						J_ASSERT(obh != NULL);
>    594						blocknr = read_tag_block(journal,
>    595									 tag);
>    596	
>    597						/* If the block has been
>    598						 * revoked, then we're all done
>    599						 * here. */
>    600						if (jbd2_journal_test_revoke
>    601						    (journal, blocknr,
>    602						     next_commit_ID)) {
>    603							brelse(obh);
>    604							++info->nr_revoke_hits;
>    605							goto skip_write;
>    606						}
>    607	
>    608						/* Look for block corruption */
>    609						if (!jbd2_block_tag_csum_verify(
>    610							journal, tag, obh->b_data,
>    611							be32_to_cpu(tmp->h_sequence))) {
>    612							brelse(obh);
>    613							success = -EFSBADCRC;
>    614							printk(KERN_ERR "JBD2: Invalid "
>    615							       "checksum recovering "
>    616							       "data block %llu in "
>    617							       "log\n", blocknr);
>    618							block_error = 1;
>    619							goto skip_write;
>    620						}
>    621	
>    622						/* Find a buffer for the new
>    623						 * data being restored */
>    624						nbh = __getblk(journal->j_fs_dev,
>    625								blocknr,
>    626								journal->j_blocksize);
>    627						if (nbh == NULL) {
>    628							printk(KERN_ERR
>    629							       "JBD2: Out of memory "
>    630							       "during recovery.\n");
>    631							err = -ENOMEM;
>    632							brelse(bh);
>    633							brelse(obh);
>    634							goto failed;
>    635						}
>    636	
>    637						lock_buffer(nbh);
>    638						memcpy(nbh->b_data, obh->b_data,
>    639								journal->j_blocksize);
>    640						if (flags & JBD2_FLAG_ESCAPE) {
>    641							*((__be32 *)nbh->b_data) =
>    642							cpu_to_be32(JBD2_MAGIC_NUMBER);
>    643						}
>    644	
>    645						BUFFER_TRACE(nbh, "marking dirty");
>    646						set_buffer_uptodate(nbh);
>    647						mark_buffer_dirty(nbh);
>    648						BUFFER_TRACE(nbh, "marking uptodate");
>    649						++info->nr_replays;
>    650						/* ll_rw_block(WRITE, 1, &nbh); */
>    651						unlock_buffer(nbh);
>    652						brelse(obh);
>    653						brelse(nbh);
>    654					}
>    655	
>    656				skip_write:
>    657					tagp += tag_bytes;
>    658					if (!(flags & JBD2_FLAG_SAME_UUID))
>    659						tagp += 16;
>    660	
>    661					if (flags & JBD2_FLAG_LAST_TAG)
>    662						break;
>    663				}
>    664	
>    665				brelse(bh);
>    666				continue;
>    667	
>    668			case JBD2_COMMIT_BLOCK:
>    669				/*     How to differentiate between interrupted commit
>    670				 *               and journal corruption ?
>    671				 *
>    672				 * {nth transaction}
>    673				 *        Checksum Verification Failed
>    674				 *			 |
>    675				 *		 ____________________
>    676				 *		|		     |
>    677				 * 	async_commit             sync_commit
>    678				 *     		|                    |
>    679				 *		| GO TO NEXT    "Journal Corruption"
>    680				 *		| TRANSACTION
>    681				 *		|
>    682				 * {(n+1)th transanction}
>    683				 *		|
>    684				 * 	 _______|______________
>    685				 * 	|	 	      |
>    686				 * Commit block found	Commit block not found
>    687				 *      |		      |
>    688				 * "Journal Corruption"       |
>    689				 *		 _____________|_________
>    690				 *     		|	           	|
>    691				 *	nth trans corrupt	OR   nth trans
>    692				 *	and (n+1)th interrupted     interrupted
>    693				 *	before commit block
>    694				 *      could reach the disk.
>    695				 *	(Cannot find the difference in above
>    696				 *	 mentioned conditions. Hence assume
>    697				 *	 "Interrupted Commit".)
>    698				 */
>    699	
>    700				/*
>    701				 * Found an expected commit block: if checksums
>    702				 * are present, verify them in PASS_SCAN; else not
>    703				 * much to do other than move on to the next sequence
>    704				 * number.
>    705				 */
>    706				if (pass == PASS_SCAN &&
>    707				    jbd2_has_feature_checksum(journal)) {
>    708					struct commit_header *cbh =
>    709						(struct commit_header *)bh->b_data;
>    710					unsigned found_chksum =
>    711						be32_to_cpu(cbh->h_chksum[0]);
>    712					__be64 commit_time =
>  > 713						be64_to_cpu(cbh->h_commit_sec);
>    714	
>    715					if (info->end_transaction) {
>    716						journal->j_failed_commit =
>    717							info->end_transaction;
>    718						brelse(bh);
>    719						break;
>    720					}
>    721	
>    722					/*
>    723					 * If need_check_commit_time is set, it means
>    724					 * csum verify failed before, if commit_time is
>    725					 * increasing, it's same journal, otherwise it
>    726					 * is stale journal block, just end this
>    727					 * recovery.
>    728					 */
>    729					if (need_check_commit_time) {
>    730						if (commit_time >= last_trans_commit_time) {
>    731							pr_err("JBD2: Invalid checksum found in transaction %u\n",
>    732							       next_commit_ID);
>    733							err = -EFSBADCRC;
>    734							brelse(bh);
>    735							goto failed;
>    736						}
>    737						/*
>    738						 * It likely does not belong to same
>    739						 * journal, just end this recovery with
>    740						 * success.
>    741						 */
>    742						jbd_debug(1, "JBD2: Invalid checksum ignored in transaction %u, likely stale data\n",
>    743							  next_commit_ID);
>    744						err = 0;
>    745						brelse(bh);
>    746						goto done;
>    747					}
>    748	
>    749					/* Neither checksum match nor unused? */
>    750					if (!((crc32_sum == found_chksum &&
>    751					       cbh->h_chksum_type ==
>    752							JBD2_CRC32_CHKSUM &&
>    753					       cbh->h_chksum_size ==
>    754							JBD2_CRC32_CHKSUM_SIZE) ||
>    755					      (cbh->h_chksum_type == 0 &&
>    756					       cbh->h_chksum_size == 0 &&
>    757					       found_chksum == 0)))
>    758						goto chksum_error;
>    759	
>    760					crc32_sum = ~0;
>    761					last_trans_commit_time = commit_time;
>    762				}
>    763				if (pass == PASS_SCAN &&
>    764				    !jbd2_commit_block_csum_verify(journal,
>    765								   bh->b_data)) {
>    766				chksum_error:
>    767					info->end_transaction = next_commit_ID;
>    768	
>    769					if (!jbd2_has_feature_async_commit(journal)) {
>    770						journal->j_failed_commit =
>    771							next_commit_ID;
>    772						brelse(bh);
>    773						break;
>    774					}
>    775				}
>    776				brelse(bh);
>    777				next_commit_ID++;
>    778				continue;
>    779	
>    780			case JBD2_REVOKE_BLOCK:
>    781				/*
>    782				 * Check revoke block crc in pass_scan, if csum verify
>    783				 * failed, check commit block time later.
>    784				 */
>    785				if (pass == PASS_SCAN &&
>    786				    !jbd2_descriptor_block_csum_verify(journal,
>    787								       bh->b_data)) {
>    788					jbd_debug(1, "JBD2: invalid revoke block found in %lu\n",
>    789						  next_log_block);
>    790					need_check_commit_time = true;
>    791				}
>    792				/* If we aren't in the REVOKE pass, then we can
>    793				 * just skip over this block. */
>    794				if (pass != PASS_REVOKE) {
>    795					brelse(bh);
>    796					continue;
>    797				}
>    798	
>    799				err = scan_revoke_records(journal, bh,
>    800							  next_commit_ID, info);
>    801				brelse(bh);
>    802				if (err)
>    803					goto failed;
>    804				continue;
>    805	
>    806			default:
>    807				jbd_debug(3, "Unrecognised magic %d, end of scan.\n",
>    808					  blocktype);
>    809				brelse(bh);
>    810				goto done;
>    811			}
>    812		}
>    813	
>    814	 done:
>    815		/*
>    816		 * We broke out of the log scan loop: either we came to the
>    817		 * known end of the log or we found an unexpected block in the
>    818		 * log.  If the latter happened, then we know that the "current"
>    819		 * transaction marks the end of the valid log.
>    820		 */
>    821	
>    822		if (pass == PASS_SCAN) {
>    823			if (!info->end_transaction)
>    824				info->end_transaction = next_commit_ID;
>    825		} else {
>    826			/* It's really bad news if different passes end up at
>    827			 * different places (but possible due to IO errors). */
>    828			if (info->end_transaction != next_commit_ID) {
>    829				printk(KERN_ERR "JBD2: recovery pass %d ended at "
>    830					"transaction %u, expected %u\n",
>    831					pass, next_commit_ID, info->end_transaction);
>    832				if (!success)
>    833					success = -EIO;
>    834			}
>    835		}
>    836		if (block_error && success == 0)
>    837			success = -EIO;
>    838		return success;
>    839	
>    840	 failed:
>    841		return err;
>    842	}
>    843	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org


-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists