* [PATCH] target: pscsi: avoid OOM in pscsi_map_sg() @ 2021-03-23 16:22 mwilck 2021-03-23 16:29 ` Christoph Hellwig 2021-03-23 18:07 ` Chaitanya Kulkarni 0 siblings, 2 replies; 6+ messages in thread From: mwilck @ 2021-03-23 16:22 UTC (permalink / raw) To: Martin K. Petersen Cc: linux-scsi, target-devel, David Disseldorp, Jürgen Groß, Martin Wilck From: Martin Wilck <mwilck@suse.com> pscsi_map_sg() uses the variable nr_pages as a hint for bio_kmalloc() how many vector elements to allocate. If nr_pages is < BIO_MAX_PAGES, it will be reset to 0 after successful allocation of the bio. If bio_add_pc_page() fails later for whatever reason, pscsi_map_sg() tries to allocate another bio, passing nr_vecs=0. This causes bio_add_pc_page() to fail immediately in the next call. pci_map_sg() continues to allocate zero-length bios until memory is exhausted and the kernel crashes with OOM. This can be easily observed by exporting a SATA DVD drive via pscsi. The target crashes as soon as the client tries to access the DVD LUN. In the case I analyzed, bio_add_pc_page() would fail because the DVD device's max_sectors_kb (128) was exceeded. Avoid this by simply not resetting nr_pages to 0 after allocating the bio. This way, the client receives an IO error when it tries to send requests exceeding the devices max_sectors_kb, and eventually gets it right. The client must still limit max_sectors_kb e.g. by an udev rule if (like in my case) the driver doesn't report valid block limits, otherwise it encounters I/O errors. Signed-off-by: Martin Wilck <mwilck@suse.com> --- drivers/target/target_core_pscsi.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c index 7b1035e..977362d 100644 --- a/drivers/target/target_core_pscsi.c +++ b/drivers/target/target_core_pscsi.c @@ -881,7 +881,6 @@ pscsi_map_sg(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, if (!bio) { new_bio: nr_vecs = bio_max_segs(nr_pages); - nr_pages -= nr_vecs; /* * Calls bio_kmalloc() and sets bio->bi_end_io() */ -- 2.26.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] target: pscsi: avoid OOM in pscsi_map_sg() 2021-03-23 16:22 [PATCH] target: pscsi: avoid OOM in pscsi_map_sg() mwilck @ 2021-03-23 16:29 ` Christoph Hellwig 2021-03-23 20:01 ` Martin Wilck 2021-03-23 18:07 ` Chaitanya Kulkarni 1 sibling, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2021-03-23 16:29 UTC (permalink / raw) To: mwilck Cc: Martin K. Petersen, linux-scsi, target-devel, David Disseldorp, J??rgen Gro?? On Tue, Mar 23, 2021 at 05:22:03PM +0100, mwilck@suse.com wrote: > Avoid this by simply not resetting nr_pages to 0 after allocating the > bio. This way, the client receives an IO error when it tries to send > requests exceeding the devices max_sectors_kb, and eventually gets > it right. The client must still limit max_sectors_kb e.g. by an udev > rule if (like in my case) the driver doesn't report valid block > limits, otherwise it encounters I/O errors. FYI, I think the what you did here is correct, but not enough. When pscsi_get_bio (that is bio_kmalloc) fails, this function needs to unwind and return an error insted of blindly retrying the allocation, else we can't recover from a memory shortage. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target: pscsi: avoid OOM in pscsi_map_sg() 2021-03-23 16:29 ` Christoph Hellwig @ 2021-03-23 20:01 ` Martin Wilck 0 siblings, 0 replies; 6+ messages in thread From: Martin Wilck @ 2021-03-23 20:01 UTC (permalink / raw) To: Christoph Hellwig Cc: Martin K. Petersen, linux-scsi, target-devel, David Disseldorp, J??rgen Gro?? On Tue, 2021-03-23 at 16:29 +0000, Christoph Hellwig wrote: > On Tue, Mar 23, 2021 at 05:22:03PM +0100, mwilck@suse.com wrote: > > Avoid this by simply not resetting nr_pages to 0 after allocating the > > bio. This way, the client receives an IO error when it tries to send > > requests exceeding the devices max_sectors_kb, and eventually gets > > it right. The client must still limit max_sectors_kb e.g. by an udev > > rule if (like in my case) the driver doesn't report valid block > > limits, otherwise it encounters I/O errors. > > FYI, I think the what you did here is correct, but not enough. > When pscsi_get_bio (that is bio_kmalloc) fails, this function needs > to unwind and return an error insted of blindly retrying the > allocation, > else we can't recover from a memory shortage. I can try to do that, but in the tests I ran, I never observed bio_kmalloc() failing. I saw it eat all memory, and various processes being killed by the OOM killer, before the system eventually panicked with OOM. It takes only fractions of a second until this happens: [ 57.356238] oom-kill:constraint=CONSTRAINT_NONE,nodemask=(null),cpuset=/,mems_allowed=0,global_oom,task_memcg=/,task=lightdm-gtk-gre,pid=4586,uid=481 [ 57.369635] Out of memory: Killed process 4586 (lightdm-gtk-gre) total-vm:1035752kB, anon-rss:0kB, file-rss:2936kB, shmem-rss:0kB ... [ 57.698656] Node 0 Normal free:54828kB min:55008kB low:68760kB high:82512kB active_anon:4kB inactive_anon:0kB active_file:936kB inactive_file:1164kB unevictable:58036kB writepending:720kB present:13631488kB managed:13310252kB mlocked:58036kB kernel_stack:5808kB pagetables:8972kB bounce:0kB free_pcp:564kB local_pcp:0kB free_cma:0kB ... [ 57.818978] Unreclaimable slab info: [ 58.254160] kmalloc-192 15683546KB 15683546KB (system has 16GiB memory). Martin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target: pscsi: avoid OOM in pscsi_map_sg() 2021-03-23 16:22 [PATCH] target: pscsi: avoid OOM in pscsi_map_sg() mwilck 2021-03-23 16:29 ` Christoph Hellwig @ 2021-03-23 18:07 ` Chaitanya Kulkarni 2021-03-23 21:20 ` Martin Wilck 1 sibling, 1 reply; 6+ messages in thread From: Chaitanya Kulkarni @ 2021-03-23 18:07 UTC (permalink / raw) To: mwilck Cc: Martin K. Petersen, linux-scsi, target-devel, David Disseldorp, Jürgen Groß Martin, On 3/23/21 09:23, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > pscsi_map_sg() uses the variable nr_pages as a hint for bio_kmalloc() > how many vector elements to allocate. If nr_pages is < BIO_MAX_PAGES, > it will be reset to 0 after successful allocation of the bio. I think BIO_MAX_PAGES is replaced by BIO_MAX_VECS with commit a8affc03a9b3 ("block: rename BIO_MAX_PAGES to BIO_MAX_VECS"). ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target: pscsi: avoid OOM in pscsi_map_sg() 2021-03-23 18:07 ` Chaitanya Kulkarni @ 2021-03-23 21:20 ` Martin Wilck 2021-03-23 21:25 ` Chaitanya Kulkarni 0 siblings, 1 reply; 6+ messages in thread From: Martin Wilck @ 2021-03-23 21:20 UTC (permalink / raw) To: Chaitanya Kulkarni, Christoph Hellwig Cc: Martin K. Petersen, linux-scsi, target-devel, David Disseldorp, Jürgen Groß On Tue, 2021-03-23 at 18:07 +0000, Chaitanya Kulkarni wrote: > Martin, > > On 3/23/21 09:23, mwilck@suse.com wrote: > > From: Martin Wilck <mwilck@suse.com> > > > > pscsi_map_sg() uses the variable nr_pages as a hint for > > bio_kmalloc() > > how many vector elements to allocate. If nr_pages is < > > BIO_MAX_PAGES, > > it will be reset to 0 after successful allocation of the bio. > > I think BIO_MAX_PAGES is replaced by BIO_MAX_VECS with > commit a8affc03a9b3 ("block: rename BIO_MAX_PAGES to BIO_MAX_VECS"). Right. I made my patch against mkp/queue, which doesn't include this commit yet. As this is just in the description, I don't think it matters much, does it? Martin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target: pscsi: avoid OOM in pscsi_map_sg() 2021-03-23 21:20 ` Martin Wilck @ 2021-03-23 21:25 ` Chaitanya Kulkarni 0 siblings, 0 replies; 6+ messages in thread From: Chaitanya Kulkarni @ 2021-03-23 21:25 UTC (permalink / raw) To: Martin Wilck, hch Cc: Martin K. Petersen, linux-scsi, target-devel, David Disseldorp, Jürgen Groß On 3/23/21 14:21, Martin Wilck wrote: >> I think BIO_MAX_PAGES is replaced by BIO_MAX_VECS with >> commit a8affc03a9b3 ("block: rename BIO_MAX_PAGES to BIO_MAX_VECS"). > Right. I made my patch against mkp/queue, which doesn't include this > commit yet. As this is just in the description, I don't think it > matters much, does it? > > Martin > > > I don't think it does, I'll let you decide that. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-23 21:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-23 16:22 [PATCH] target: pscsi: avoid OOM in pscsi_map_sg() mwilck 2021-03-23 16:29 ` Christoph Hellwig 2021-03-23 20:01 ` Martin Wilck 2021-03-23 18:07 ` Chaitanya Kulkarni 2021-03-23 21:20 ` Martin Wilck 2021-03-23 21:25 ` Chaitanya Kulkarni
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.