Skip to content

Commit

Permalink
iolog: fix double free when two in-flight verified IOs overlap
Browse files Browse the repository at this point in the history
When running
valgrind ./fio --randseed=1 --ioengine=posixaio --thread --rw=randrw \
 --random_distribution=zipf:1.4 --filename=/tmp/fiofile --io_limit=50M \
 --verify=crc32c --name=verifyfree --iodepth=32 --bs=64k --size=100M

valgrind reports:
==21982== Invalid read of size 4
==21982==    at 0x442DBC: io_completed (io_u.c:1835)
==21982==    by 0x442DBC: ios_completed (io_u.c:1924)
==21982==    by 0x442DBC: io_u_queued_complete (io_u.c:1983)
==21982==    by 0x40D2A5: wait_for_completions (backend.c:455)
==21982==    by 0x465041: do_io (backend.c:1003)
==21982==    by 0x465041: thread_main (backend.c:1703)
==21982==    by 0x576E6B9: start_thread (pthread_create.c:333)
==21982==    by 0x5C8E82C: clone (clone.S:109)
==21982==  Address 0x62cf7c8 is 72 bytes inside a block of size 88 free'd
==21982==    at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21982==    by 0x45F461: log_io_piece (iolog.c:286)
[...]

When __ipo is still in-flight we can't free it when it overlaps because it will
be used at I/O completion time so add it to an orphan ipo list which is freed
when the job finishes.

Fixes axboe#336 .

Signed-off-by: Sitsofe Wheeler <sitsofe@yahoo.com>
  • Loading branch information
sitsofe committed May 8, 2017
1 parent c55fae0 commit 6b4cfeb
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 8 deletions.
13 changes: 13 additions & 0 deletions backend.c
Original file line number Diff line number Diff line change
Expand Up @@ -1142,6 +1142,17 @@ static void cleanup_io_u(struct thread_data *td)
free_file_completion_logging(td);
}

static void cleanup_ipo_orphans(struct thread_data *td)
{
struct io_piece *ipo;

while (!flist_empty(&td->io_orphan_list)) {
ipo = flist_first_entry(&td->io_orphan_list, struct io_piece, list);
flist_del(&ipo->list);
free(ipo);
}
}

static int init_io_u(struct thread_data *td)
{
struct io_u *io_u;
Expand Down Expand Up @@ -1483,6 +1494,7 @@ static void *thread_main(void *data)
INIT_FLIST_HEAD(&td->verify_list);
INIT_FLIST_HEAD(&td->trim_list);
INIT_FLIST_HEAD(&td->next_rand_list);
INIT_FLIST_HEAD(&td->io_orphan_list);
td->io_hist_tree = RB_ROOT;

ret = mutex_cond_init_pshared(&td->io_u_lock, &td->free_cond);
Expand Down Expand Up @@ -1825,6 +1837,7 @@ static void *thread_main(void *data)

close_and_free_files(td);
cleanup_io_u(td);
cleanup_ipo_orphans(td);
close_ioengine(td);
cgroup_shutdown(td, &cgroup_mnt);
verify_free_state(td);
Expand Down
1 change: 1 addition & 0 deletions fio.h
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,7 @@ struct thread_data {
*/
struct rb_root io_hist_tree;
struct flist_head io_hist_list;
struct flist_head io_orphan_list;
unsigned long io_hist_len;

/*
Expand Down
25 changes: 17 additions & 8 deletions iolog.c
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,14 @@ void log_io_piece(struct thread_data *td, struct io_u *io_u)
td->io_hist_len--;
rb_erase(parent, &td->io_hist_tree);
remove_trim_entry(td, __ipo);
free(__ipo);
if (__ipo->flags & IP_F_IN_FLIGHT) {
__ipo->flags &= ~IP_F_ONRB;
__ipo->flags |= IP_F_UNLOGGED;
__ipo->flags |= IP_F_ONLIST;
flist_add_tail(&__ipo->list, &td->io_orphan_list);
write_barrier();
} else
free(__ipo);
goto restart;
}
}
Expand Down Expand Up @@ -313,14 +320,16 @@ void unlog_io_piece(struct thread_data *td, struct io_u *io_u)
if (!ipo)
return;

if (ipo->flags & IP_F_ONRB)
rb_erase(&ipo->rb_node, &td->io_hist_tree);
else if (ipo->flags & IP_F_ONLIST)
flist_del(&ipo->list);
if (!(ipo->flags & IP_F_UNLOGGED)) {
if (ipo->flags & IP_F_ONRB)
rb_erase(&ipo->rb_node, &td->io_hist_tree);
else if (ipo->flags & IP_F_ONLIST)
flist_del(&ipo->list);

free(ipo);
io_u->ipo = NULL;
td->io_hist_len--;
free(ipo);
io_u->ipo = NULL;
td->io_hist_len--;
}
}

void trim_io_piece(struct thread_data *td, const struct io_u *io_u)
Expand Down
1 change: 1 addition & 0 deletions iolog.h
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ enum {
IP_F_ONLIST = 2,
IP_F_TRIMMED = 4,
IP_F_IN_FLIGHT = 8,
IP_F_UNLOGGED = 16,
};

/*
Expand Down
1 change: 1 addition & 0 deletions rate-submit.c
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ static int io_workqueue_init_worker_fn(struct submit_worker *sw)
INIT_FLIST_HEAD(&td->verify_list);
INIT_FLIST_HEAD(&td->trim_list);
INIT_FLIST_HEAD(&td->next_rand_list);
INIT_FLIST_HEAD(&td->io_orphan_list);
td->io_hist_tree = RB_ROOT;

td->o.iodepth = 1;
Expand Down

0 comments on commit 6b4cfeb

Please sign in to comment.