| Message ID | 20200428032745.133556-3-jannh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix ELF / FDPIC ELF core dumping, and use mmap_sem properly in there | expand |
On Mon, Apr 27, 2020 at 8:28 PM Jann Horn <jannh@google.com> wrote: > > After a partial write, we have to update the input buffer pointer. Interesting. It seems this partial write case never triggers (except for actually killing the core-dump). Or did you find a case where it actually matters? Your fix is obviously correct, but it also makes me go "that function clearly never actually worked for partial writes, maybe we shouldn't even bother?" Linus
On Tue, Apr 28, 2020 at 5:36 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Mon, Apr 27, 2020 at 8:28 PM Jann Horn <jannh@google.com> wrote: > > > > After a partial write, we have to update the input buffer pointer. > > Interesting. It seems this partial write case never triggers (except > for actually killing the core-dump). > > Or did you find a case where it actually matters? > > Your fix is obviously correct, but it also makes me go "that function > clearly never actually worked for partial writes, maybe we shouldn't > even bother?" Hmm, yeah... I can't really think of cases where write handlers can spuriously return early without having a pending signal, and a second write is likely to succeed... I just know that there are some things that are notorious for returning short *reads* (e.g. pipes, sockets, /proc/$pid/maps). Al's commit message refers to pipes specifically; but even at commit 2507a4fbd48a, I don't actually see where pipe_write() could return a short write without a page allocation failure or something like that. So maybe you're right and we should just get rid of it...
On Tue, Apr 28, 2020 at 9:34 AM Rob Landley <rob@landley.net> wrote: > > Writes to a local filesystem should never be short unless disk full/error. Well, that code is definitely supposed to also write to pipes. But it also has "was I interrupted" logic, which stops the core dump. So short writes can very much happen, it's just that they also imply that the core dump should be aborted. So the loop seems to be unnecessary. The situations where short writes can happen are all the same situations where we want to abort anyway, so the loop count should probably always be just one. The same would go for any potential network filesystem with the traditional NFS intr-like behavior. Linus
On 4/27/20 10:35 PM, Linus Torvalds wrote: > On Mon, Apr 27, 2020 at 8:28 PM Jann Horn <jannh@google.com> wrote: >> >> After a partial write, we have to update the input buffer pointer. > > Interesting. It seems this partial write case never triggers (except > for actually killing the core-dump). > > Or did you find a case where it actually matters? > > Your fix is obviously correct, but it also makes me go "that function > clearly never actually worked for partial writes, maybe we shouldn't > even bother?" Writes to a local filesystem should never be short unless disk full/error. Once upon a time this was yet another thing that NFS could break that no other filesystem would break, but I dunno about now? (I think the page cache collates it and defers the flush until the error can't be reported back anyway?) Rob
diff --git a/fs/coredump.c b/fs/coredump.c index 408418e6aa131..047f5a11dbee7 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -833,6 +833,7 @@ int dump_emit(struct coredump_params *cprm, const void *addr, int nr) cprm->written += n; cprm->pos += n; nr -= n; + addr += n; } return 1; }
After a partial write, we have to update the input buffer pointer. Fixes: 2507a4fbd48a ("make dump_emit() use vfs_write() instead of banging at ->f_op->write directly") Cc: stable@vger.kernel.org Signed-off-by: Jann Horn <jannh@google.com> --- fs/coredump.c | 1 + 1 file changed, 1 insertion(+)