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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.