Skip to contentSkip to conversation

Coverity: add_ipu_page(): Memory - illegal accesses

DISCUSSION coverity-bot 2 participants 3 messages Nov 12, 2019 Linux-next tree

lore
Share & tools
lore
#0 lore raw

Hello!

This is an experimental automated report about issues detected by Coverity from a scan of next-20191108 as part of the linux-next weekly scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan

You're getting this email because you were associated with the identified lines of code (noted below) that were touched by recent commits:

0b20fcec8651 ("f2fs: cache global IPU bio")

Coverity reported the following:

*** CID 1487851: Memory - illegal accesses (USE_AFTER_FREE) /fs/f2fs/data.c: 604 in add_ipu_page()

598     			break;
599     		}
600     		up_write(&io->bio_list_lock);
601     	}
602
603     	if (ret) {
vvv     CID 1487851:  Memory - illegal accesses  (USE_AFTER_FREE)
vvv     Calling "bio_put" dereferences freed pointer "*bio".
604     		bio_put(*bio);
605     		*bio = NULL;
606     	}

607
608 return ret;
609 }

If this is a false positive, please let us know so we can mark it as such, or teach the Coverity rules to be smarter. If not, please make sure fixes get into linux-next. :) For patches fixing this, please include these lines (but double-check the "Fixes" first):

Addresses-Coverity-ID: 1487851 ("Memory - illegal accesses")

Fixes 0b20fcec8651 ("f2fs: cache global IPU bio")

Thanks for your attention!

Signature · 1 line
Coverity-bot
#1 lore raw
Quoted 26 lines · coverity-bot

Hello!

This is an experimental automated report about issues detected by Coverity from a scan of next-20191108 as part of the linux-next weekly scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan

You're getting this email because you were associated with the identified lines of code (noted below) that were touched by recent commits:

0b20fcec8651 ("f2fs: cache global IPU bio")

Coverity reported the following:

*** CID 1487851: Memory - illegal accesses (USE_AFTER_FREE) /fs/f2fs/data.c: 604 in add_ipu_page()

598     			break;
599     		}
600     		up_write(&io->bio_list_lock);
601     	}
602
603     	if (ret) {
vvv     CID 1487851:  Memory - illegal accesses  (USE_AFTER_FREE)
vvv     Calling "bio_put" dereferences freed pointer "*bio".
604     		bio_put(*bio);
605     		*bio = NULL;
606     	}
Quoted 3 lines

607
608 return ret;
609 }

Thanks for the report.

I double check these related codes:

static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
struct page *page)

{
	enum temp_type temp;
	bool found = false;
	int ret = -EAGAIN;

	for (temp = HOT; temp < NR_TEMP_TYPE && !found; temp++) {
		struct f2fs_bio_info *io = sbi->write_io[DATA] + temp;
		struct list_head *head = &io->bio_list;
		struct bio_entry *be;

		down_write(&io->bio_list_lock);
		list_for_each_entry(be, head, list) {
			if (be->bio != *bio)
				continue;

			found = true;

			if (bio_add_page(*bio, page, PAGE_SIZE, 0) == PAGE_SIZE) {
				ret = 0;
				break;
			}

			/* bio is full */
			del_bio_entry(be);
			__submit_bio(sbi, *bio, DATA);
			break;
		}
		up_write(&io->bio_list_lock);
	}

	if (ret) {

If we get here, that means 1) found nothing due to someone has submitted bio for us, or 2) found target bio, however bio is full, we submitted the bio. For both conditions, previously, we grab one extra ref on bio, here, we just release the ref and reset *bio to NULL, then caller can allocate new bio.

Let me know if I'm missing something.

		bio_put(*bio);
		*bio = NULL;
	}

	return ret;
}
Quoted 13 lines

If this is a false positive, please let us know so we can mark it as such, or teach the Coverity rules to be smarter. If not, please make sure fixes get into linux-next. :) For patches fixing this, please include these lines (but double-check the "Fixes" first):

Addresses-Coverity-ID: 1487851 ("Memory - illegal accesses")

Fixes 0b20fcec8651 ("f2fs: cache global IPU bio")

Thanks for your attention!

author
#2 lore raw
Quoted 69 lines · Chao Yu
Quoted 26 lines · coverity-bot

Hello!

This is an experimental automated report about issues detected by Coverity from a scan of next-20191108 as part of the linux-next weekly scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan

You're getting this email because you were associated with the identified lines of code (noted below) that were touched by recent commits:

0b20fcec8651 ("f2fs: cache global IPU bio")

Coverity reported the following:

*** CID 1487851: Memory - illegal accesses (USE_AFTER_FREE) /fs/f2fs/data.c: 604 in add_ipu_page()

598     			break;
599     		}
600     		up_write(&io->bio_list_lock);
601     	}
602
603     	if (ret) {
vvv     CID 1487851:  Memory - illegal accesses  (USE_AFTER_FREE)
vvv     Calling "bio_put" dereferences freed pointer "*bio".
604     		bio_put(*bio);
605     		*bio = NULL;
606     	}
Quoted 3 lines

607
608 return ret;
609 }

Thanks for the report.

I double check these related codes:

static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
struct page *page)

{
	enum temp_type temp;
	bool found = false;
	int ret = -EAGAIN;

	for (temp = HOT; temp < NR_TEMP_TYPE && !found; temp++) {
		struct f2fs_bio_info *io = sbi->write_io[DATA] + temp;
		struct list_head *head = &io->bio_list;
		struct bio_entry *be;

		down_write(&io->bio_list_lock);
		list_for_each_entry(be, head, list) {
			if (be->bio != *bio)
				continue;

			found = true;

			if (bio_add_page(*bio, page, PAGE_SIZE, 0) == PAGE_SIZE) {
				ret = 0;
				break;
			}

			/* bio is full */
			del_bio_entry(be);
			__submit_bio(sbi, *bio, DATA);
			break;
		}
		up_write(&io->bio_list_lock);
	}

	if (ret) {
Quoted 6 lines

If we get here, that means 1) found nothing due to someone has submitted bio for us, or 2) found target bio, however bio is full, we submitted the bio. For both conditions, previously, we grab one extra ref on bio, here, we just release the ref and reset *bio to NULL, then caller can allocate new bio.

Let me know if I'm missing something.

Okay, I've noted it as a false positive. I don't know this code at all, so I can't really comment on the lifetime expectations here. :)

Thanks for looking at it!

-Kees

Quoted 21 lines
		bio_put(*bio);
		*bio = NULL;
	}

	return ret;
}
Quoted 13 lines

If this is a false positive, please let us know so we can mark it as such, or teach the Coverity rules to be smarter. If not, please make sure fixes get into linux-next. :) For patches fixing this, please include these lines (but double-check the "Fixes" first):

Addresses-Coverity-ID: 1487851 ("Memory - illegal accesses")

Fixes 0b20fcec8651 ("f2fs: cache global IPU bio")

Thanks for your attention!

Signature · 1 line
Kees Cook