Messages in this thread Patches in this message | | | Date | Wed, 19 Apr 2023 15:05:22 +0200 | Subject | Re: [PATCH v3 03/11] documentation: Block Devices Snapshots Module | From | Sergei Shtepa <> |
| |
On 4/18/23 16:48, Donald Buczek wrote: > Subject: > Re: [PATCH v3 03/11] documentation: Block Devices Snapshots Module > From: > Donald Buczek <buczek@molgen.mpg.de> > Date: > 4/18/23, 16:48 > > To: > Sergei Shtepa <sergei.shtepa@veeam.com>, axboe@kernel.dk, hch@infradead.org, corbet@lwn.net, snitzer@kernel.org > CC: > viro@zeniv.linux.org.uk, brauner@kernel.org, willy@infradead.org, kch@nvidia.com, martin.petersen@oracle.com, vkoul@kernel.org, ming.lei@redhat.com, gregkh@linuxfoundation.org, linux-block@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org > > > On 4/18/23 12:31, Sergei Shtepa wrote: >> >> >> On 4/14/23 14:34, Sergei Shtepa wrote: >>> Subject: >>> Re: [PATCH v3 03/11] documentation: Block Devices Snapshots Module >>> From: >>> Sergei Shtepa <sergei.shtepa@veeam.com> >>> Date: >>> 4/14/23, 14:34 >>> >>> To: >>> Donald Buczek <buczek@molgen.mpg.de>, axboe@kernel.dk, hch@infradead.org, corbet@lwn.net, snitzer@kernel.org >>> CC: >>> viro@zeniv.linux.org.uk, brauner@kernel.org, willy@infradead.org, kch@nvidia.com, martin.petersen@oracle.com, vkoul@kernel.org, ming.lei@redhat.com, gregkh@linuxfoundation.org, linux-block@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org >>> >>> >>> >>> On 4/12/23 21:38, Donald Buczek wrote: >>>> Subject: >>>> Re: [PATCH v3 03/11] documentation: Block Devices Snapshots Module >>>> From: >>>> Donald Buczek <buczek@molgen.mpg.de> >>>> Date: >>>> 4/12/23, 21:38 >>>> >>>> To: >>>> Sergei Shtepa <sergei.shtepa@veeam.com>, axboe@kernel.dk, hch@infradead.org, corbet@lwn.net, snitzer@kernel.org >>>> CC: >>>> viro@zeniv.linux.org.uk, brauner@kernel.org, willy@infradead.org, kch@nvidia.com, martin.petersen@oracle.com, vkoul@kernel.org, ming.lei@redhat.com, gregkh@linuxfoundation.org, linux-block@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org >>>> >>>> >>>> I think, you can trigger all kind of user-after-free when userspace deletes a snapshot image or the snapshot image and the tracker while the disk device snapshot image is kept alive (mounted or just opened) and doing I/O. >>>> >>>> Here is what I did to provoke that: >>>> >>>> root@dose:~# s=$(blksnap snapshot_create -d /dev/vdb) >>>> root@dose:~# blksnap snapshot_appendstorage -i $s -f /scratch/local/test.dat >>>> device path: '/dev/block/253:2' >>>> allocate range: ofs=11264624 cnt=2097152 >>>> root@dose:~# blksnap snapshot_take -i $s >>>> root@dose:~# mount /dev/blksnap-image_253\:16 /mnt >>>> root@dose:~# dd if=/dev/zero of=/mnt/x.x & >>>> [1] 2514 >>>> root@dose:~# blksnap snapshot_destroy -i $s >>>> dd: writing to '/mnt/x.x': No space left on device >>>> 1996041+0 records in >>>> 1996040+0 records out >>>> 1021972480 bytes (1.0 GB, 975 MiB) copied, 8.48923 s, 120 MB/s >>>> [1]+ Exit 1 dd if=/dev/zero of=/mnt/x.x >>>> >>> Thanks! >>> I am very glad that the blksnap tool turned out to be useful in the review. >>> This snapshot deletion scenario is not the most typical, but of course it is >>> quite possible. >>> I will need to solve this problem and add such a scenario to the test suite. >>> >> >> Hi! >> >> I have redesign the logic of ownership of the diff_area structure. >> See patch in attach or commit. >> Link: https://github.com/SergeiShtepa/linux/commit/7e927c381dcd2b2293be8315897a224d111b6f88 >> A test script for such a scenario has been added. >> Link: https://github.com/veeam/blksnap/commit/fd0559dfedf094901d08bbf185fed288f0156433 >> >> I will be glad of any feedback. > > Great, Thanks! > > However, there are two leftover calls to diff_area_free() with its old prototype: > > CC [M] drivers/block/blksnap/diff_area.o > drivers/block/blksnap/diff_area.c: In function ‘diff_area_new’: > drivers/block/blksnap/diff_area.c:283:18: error: passing argument 1 of ‘diff_area_free’ from incompatible pointer type [-Werror=incompatible-pointer-types] > 283 | diff_area_free(diff_area); > | ^~~~~~~~~ > | | > | struct diff_area * > drivers/block/blksnap/diff_area.c:110:34: note: expected ‘struct kref *’ but argument is of type ‘struct diff_area *’ > 110 | void diff_area_free(struct kref *kref) > | ~~~~~~~~~~~~~^~~~ > cc1: some warnings being treated as errors > make[4]: *** [scripts/Makefile.build:252: drivers/block/blksnap/diff_area.o] Error 1 > make[3]: *** [scripts/Makefile.build:494: drivers/block/blksnap] Error 2 > make[2]: *** [scripts/Makefile.build:494: drivers/block] Error 2 > make[1]: *** [scripts/Makefile.build:494: drivers] Error 2 > make: *** [Makefile:2025: .] Error 2 > > The other one: > > buczek@dose:/scratch/local/linux (blksnap-test)$ make drivers/block/blksnap/tracker.o > CALL scripts/checksyscalls.sh > DESCEND objtool > INSTALL libsubcmd_headers > CC [M] drivers/block/blksnap/tracker.o > drivers/block/blksnap/tracker.c: In function ‘tracker_free’: > drivers/block/blksnap/tracker.c:26:25: error: passing argument 1 of ‘diff_area_free’ from incompatible pointer type [-Werror=incompatible-pointer-types] > 26 | diff_area_free(tracker->diff_area); > | ~~~~~~~^~~~~~~~~~~ > | | > | struct diff_area * > In file included from drivers/block/blksnap/tracker.c:12: > drivers/block/blksnap/diff_area.h:116:34: note: expected ‘struct kref *’ but argument is of type ‘struct diff_area *’ > 116 | void diff_area_free(struct kref *kref); > | ~~~~~~~~~~~~~^~~~ > cc1: some warnings being treated as errors > make[4]: *** [scripts/Makefile.build:252: drivers/block/blksnap/tracker.o] Error 1 > make[3]: *** [scripts/Makefile.build:494: drivers/block/blksnap] Error 2 > make[2]: *** [scripts/Makefile.build:494: drivers/block] Error 2 > make[1]: *** [scripts/Makefile.build:494: drivers] Error 2 > make: *** [Makefile:2025: .] Error 2 > > Am I missing something?
Thanks!
It seems to me that I missed something. The biggest mystery for me is why I was able to build and test the kernel. I think it's some kind of incremental build effect. I was only able to see the problem after 'make clean'.
Patches in attach and https://github.com/SergeiShtepa/linux/tree/blksnap-masterdiff --git a/drivers/block/blksnap/diff_area.h b/drivers/block/blksnap/diff_area.h index cb3e09809c0f..883117dba762 100644 --- a/drivers/block/blksnap/diff_area.h +++ b/drivers/block/blksnap/diff_area.h @@ -138,6 +138,5 @@ bool diff_area_cow(struct bio *bio, struct diff_area *diff_area, bool diff_area_submit_chunk(struct diff_area *diff_area, struct bio *bio); void diff_area_rw_chunk(struct kref *kref); -//void diff_area_throttling_io(struct diff_area *diff_area); #endif /* __BLKSNAP_DIFF_AREA_H */diff --git a/drivers/block/blksnap/diff_area.c b/drivers/block/blksnap/diff_area.c index 5a3f4d74a92f..0e530d24d563 100644 --- a/drivers/block/blksnap/diff_area.c +++ b/drivers/block/blksnap/diff_area.c @@ -280,7 +280,7 @@ struct diff_area *diff_area_new(dev_t dev_id, struct diff_storage *diff_storage) } if (ret) { - diff_area_free(diff_area); + diff_area_put(diff_area); return ERR_PTR(ret); } diff --git a/drivers/block/blksnap/tracker.c b/drivers/block/blksnap/tracker.c index 6d2c77e4c90f..d98048dc5bed 100644 --- a/drivers/block/blksnap/tracker.c +++ b/drivers/block/blksnap/tracker.c @@ -23,7 +23,7 @@ void tracker_free(struct kref *kref) MINOR(tracker->dev_id)); if (tracker->diff_area) - diff_area_free(tracker->diff_area); + diff_area_put(tracker->diff_area); if (tracker->cbt_map) cbt_map_destroy(tracker->cbt_map);
| |