[ Sorry David, the complain script uses git blame and it just picked
you. And then I decided to keep the address because your email is
still active and you seem like an expert. -dan ]
Hello David Sterba,
The patch a1ee73626844: "btrfs: do an allocation earlier during
snapshot creation" from Nov 10, 2015, leads to the following static
checker warning:
fs/btrfs/ioctl.c:890 create_snapshot()
warn: 'pending_snapshot' not removed from list
fs/btrfs/ioctl.c
809
810 pending_snapshot = kzalloc(sizeof(*pending_snapshot),
GFP_KERNEL);
^^^^^^^^^^^^^^^^
Allocated here.
811 if (!pending_snapshot)
812 return -ENOMEM;
813
814 ret = get_anon_bdev(&pending_snapshot->anon_dev);
815 if (ret < 0)
816 goto free_pending;
817 pending_snapshot->root_item = kzalloc(sizeof(struct
btrfs_root_item),
818 GFP_KERNEL);
819 pending_snapshot->path = btrfs_alloc_path();
820 if (!pending_snapshot->root_item || !pending_snapshot->path) {
821 ret = -ENOMEM;
822 goto free_pending;
823 }
824
825 btrfs_init_block_rsv(&pending_snapshot->block_rsv,
826 BTRFS_BLOCK_RSV_TEMP);
827 /*
828 * 1 - parent dir inode
829 * 2 - dir entries
830 * 1 - root item
831 * 2 - root ref/backref
832 * 1 - root of snapshot
833 * 1 - UUID item
834 */
835 ret = btrfs_subvolume_reserve_metadata(BTRFS_I(dir)->root,
836 &pending_snapshot->block_rsv, 8,
837 false);
838 if (ret)
839 goto free_pending;
840
841 pending_snapshot->dentry = dentry;
842 pending_snapshot->root = root;
843 pending_snapshot->readonly = readonly;
844 pending_snapshot->dir = dir;
845 pending_snapshot->inherit = inherit;
846
847 trans = btrfs_start_transaction(root, 0);
848 if (IS_ERR(trans)) {
849 ret = PTR_ERR(trans);
850 goto fail;
851 }
852
853 spin_lock(&fs_info->trans_lock);
854 list_add(&pending_snapshot->list,
855 &trans->transaction->pending_snapshots);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Added to the list here.
856 spin_unlock(&fs_info->trans_lock);
857
858 ret = btrfs_commit_transaction(trans);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This will clean empty the ->pending_snapshots list when we call
create_pending_snapshots() but it's possible to fail before that
happens.
Possibly one fix would be to clear out the ->pending_snapshots list in
the __btrfs_end_transaction() function.
859 if (ret)
860 goto fail;
861
862 ret = pending_snapshot->error;
863 if (ret)
864 goto fail;
865
866 ret = btrfs_orphan_cleanup(pending_snapshot->snap);
867 if (ret)
868 goto fail;
869
870 inode = btrfs_lookup_dentry(d_inode(dentry->d_parent), dentry);
871 if (IS_ERR(inode)) {
872 ret = PTR_ERR(inode);
873 goto fail;
874 }
875
876 d_instantiate(dentry, inode);
877 ret = 0;
878 pending_snapshot->anon_dev = 0;
879 fail:
880 /* Prevent double freeing of anon_dev */
881 if (ret && pending_snapshot->snap)
882 pending_snapshot->snap->anon_dev = 0;
883 btrfs_put_root(pending_snapshot->snap);
884 btrfs_subvolume_release_metadata(root,
&pending_snapshot->block_rsv);
885 free_pending:
886 if (pending_snapshot->anon_dev)
887 free_anon_bdev(pending_snapshot->anon_dev);
888 kfree(pending_snapshot->root_item);
889 btrfs_free_path(pending_snapshot->path);
890 kfree(pending_snapshot);
^^^^^^^^^^^^^^^^^^^^^^^
If btrfs_commit_transaction() fails too early then this is freed but
it's still in the ->pending_snapshots list leading to a use after free.
891
892 return ret;
893 }
regards,
dan carpenter
