From: Dan Carpenter <dan.carpenter@oracle.com>
To: xfs@oss.sgi.com
Subject: potential use after free in xfs_iomap_write_allocate()
Date: Mon, 10 Feb 2014 13:36:26 +0300 [thread overview]
Message-ID: <20140210103626.GA15018@elgon.mountain> (raw)
There is a static checker warning in xfs_iomap_write_allocate(). It's
sort of old so probably it's a false positive.
fs/xfs/xfs_iomap.c:798 xfs_iomap_write_allocate()
warn: 'tp' was already freed.
fs/xfs/xfs_iomap.c
677
678 while (count_fsb != 0) {
There are some paths where if (count_fsb == 0) then "tp" is free.
679 /*
680 * Set up a transaction with which to allocate the
681 * backing store for the file. Do allocations in a
682 * loop until we get some space in the range we are
683 * interested in. The other space that might be allocated
684 * is in the delayed allocation extent on which we sit
685 * but before our buffer starts.
686 */
687
688 nimaps = 0;
689 while (nimaps == 0) {
690 tp = xfs_trans_alloc(mp, XFS_TRANS_STRAT_WRITE);
691 tp->t_flags |= XFS_TRANS_RESERVE;
692 nres = XFS_EXTENTADD_SPACE_RES(mp, XFS_DATA_FORK);
693 error = xfs_trans_reserve(tp, &M_RES(mp)->tr_write,
694 nres, 0);
695 if (error) {
696 xfs_trans_cancel(tp, 0);
697 return XFS_ERROR(error);
698 }
699 xfs_ilock(ip, XFS_ILOCK_EXCL);
700 xfs_trans_ijoin(tp, ip, 0);
701
702 xfs_bmap_init(&free_list, &first_block);
703
704 /*
705 * it is possible that the extents have changed since
706 * we did the read call as we dropped the ilock for a
707 * while. We have to be careful about truncates or hole
708 * punchs here - we are not allowed to allocate
709 * non-delalloc blocks here.
710 *
711 * The only protection against truncation is the pages
712 * for the range we are being asked to convert are
713 * locked and hence a truncate will block on them
714 * first.
715 *
716 * As a result, if we go beyond the range we really
717 * need and hit an delalloc extent boundary followed by
718 * a hole while we have excess blocks in the map, we
719 * will fill the hole incorrectly and overrun the
720 * transaction reservation.
721 *
722 * Using a single map prevents this as we are forced to
723 * check each map we look for overlap with the desired
724 * range and abort as soon as we find it. Also, given
725 * that we only return a single map, having one beyond
726 * what we can return is probably a bit silly.
727 *
728 * We also need to check that we don't go beyond EOF;
729 * this is a truncate optimisation as a truncate sets
730 * the new file size before block on the pages we
731 * currently have locked under writeback. Because they
732 * are about to be tossed, we don't need to write them
733 * back....
734 */
735 nimaps = 1;
736 end_fsb = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
737 error = xfs_bmap_last_offset(NULL, ip, &last_block,
738 XFS_DATA_FORK);
739 if (error)
740 goto trans_cancel;
741
742 last_block = XFS_FILEOFF_MAX(last_block, end_fsb);
743 if ((map_start_fsb + count_fsb) > last_block) {
744 count_fsb = last_block - map_start_fsb;
745 if (count_fsb == 0) {
746 error = EAGAIN;
747 goto trans_cancel;
748 }
749 }
750
751 /*
752 * From this point onwards we overwrite the imap
753 * pointer that the caller gave to us.
754 */
755 error = xfs_bmapi_write(tp, ip, map_start_fsb,
756 count_fsb,
757 XFS_BMAPI_STACK_SWITCH,
758 &first_block, 1,
759 imap, &nimaps, &free_list);
760 if (error)
761 goto trans_cancel;
762
763 error = xfs_bmap_finish(&tp, &free_list, &committed);
764 if (error)
765 goto trans_cancel;
766
767 error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
768 if (error)
769 goto error0;
The call to xfs_trans_commit() frees "tp".
770
771 xfs_iunlock(ip, XFS_ILOCK_EXCL);
772 }
773
774 /*
775 * See if we were able to allocate an extent that
776 * covers at least part of the callers request
777 */
778 if (!(imap->br_startblock || XFS_IS_REALTIME_INODE(ip)))
779 return xfs_alert_fsblock_zero(ip, imap);
780
781 if ((offset_fsb >= imap->br_startoff) &&
782 (offset_fsb < (imap->br_startoff +
783 imap->br_blockcount))) {
784 XFS_STATS_INC(xs_xstrat_quick);
785 return 0;
786 }
787
788 /*
789 * So far we have not mapped the requested part of the
790 * file, just surrounding data, try again.
791 */
792 count_fsb -= imap->br_blockcount;
793 map_start_fsb = imap->br_startoff + imap->br_blockcount;
794 }
795
796 trans_cancel:
797 xfs_bmap_cancel(&free_list);
798 xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
^^
We dereference "tp" in xfs_trans_cancel().
regards,
dan carpenter
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
next reply other threads:[~2014-02-10 10:36 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-10 10:36 Dan Carpenter [this message]
2014-02-10 14:21 ` potential use after free in xfs_iomap_write_allocate() Jeff Liu
2014-02-10 14:50 ` Dan Carpenter
2014-02-10 21:34 ` Dave Chinner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140210103626.GA15018@elgon.mountain \
--to=dan.carpenter@oracle.com \
--cc=xfs@oss.sgi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox