Re: [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
On Fri, Oct 26, 2018 at 08:42:12PM +0100, Kees Cook wrote: > On Fri, Oct 26, 2018 at 7:22 PM, Joel Fernandes > wrote: > > On Fri, Oct 26, 2018 at 07:16:28PM +0100, Kees Cook wrote: > >> On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google) > >> wrote: > >> > This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf. > >> > > >> > Due to the commit which is being reverted here, it is not possible to > >> > know if pstore's messages were from a previous boot, or from very old > >> > boots. This creates an awkard situation where its unclear if crash or > >> > other logs are from the previous boot or from very old boots. Also > >> > typically we dump the pstore buffers after one reboot and are interested > >> > in only the previous boot's crash so let us reset the buffer after we > >> > save them. > >> > > >> > Lastly, if we don't zap them, then I think it is possible that part of > >> > the buffer will be from this boot and the other parts will be from > >> > previous boots. So this revert fixes all of this by calling > >> > persistent_ram_zap always. > >> > >> I like the other patches (comments coming), but not this one: it's > >> very intentional to keep all crashes around until they're explicitly > >> unlinked from the pstore filesystem from userspace. Especially true > >> for catching chains of kernel crashes, or a failed log collection, > >> etc. Surviving multiple reboots is the expected behavior on Chrome OS > >> too. > > > > Oh, ok. Hence the RFC tag ;-) We can drop this one then. I forgot that > > unlinking was another way to clear the logs. > > In another thread I discovered that the "single prz" ones actually > _are_ zapped at boot. I didn't realize, but it explains why pmsg would > vanish on me sometimes. ;) I always thought I was just doing something > wrong with it. (And I wonder if it's actually a bug that pmsg is > zapped -- console doesn't matter: it's overwritten every boot by > design.) Oh yeah they are. So seems like some are zapped on boot and some aren't then. Hmm, I would think it makes sense not to boot-zap dmesg ever, since that's crash logs someone may want to see after many reboots. But console and pmsg should be since those just "what happened on the last boot". I guess it should be made clear in some structure or something which types are zapped on boot, and which ones aren't. That'll make it clear when adding a new type about that behavior, instead of relying on the assumption that single prz are zapped and multiple ones aren't. Like for ftrace, since the per-cpu configuration was added, it will now be zapped on boot if it is using a per-cpu configuration and not zapped on boot if it isn't right? That would seem a bit inconsistent. thanks! -Joel
Re: [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
On Fri, Oct 26, 2018 at 08:42:12PM +0100, Kees Cook wrote: > On Fri, Oct 26, 2018 at 7:22 PM, Joel Fernandes > wrote: > > On Fri, Oct 26, 2018 at 07:16:28PM +0100, Kees Cook wrote: > >> On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google) > >> wrote: > >> > This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf. > >> > > >> > Due to the commit which is being reverted here, it is not possible to > >> > know if pstore's messages were from a previous boot, or from very old > >> > boots. This creates an awkard situation where its unclear if crash or > >> > other logs are from the previous boot or from very old boots. Also > >> > typically we dump the pstore buffers after one reboot and are interested > >> > in only the previous boot's crash so let us reset the buffer after we > >> > save them. > >> > > >> > Lastly, if we don't zap them, then I think it is possible that part of > >> > the buffer will be from this boot and the other parts will be from > >> > previous boots. So this revert fixes all of this by calling > >> > persistent_ram_zap always. > >> > >> I like the other patches (comments coming), but not this one: it's > >> very intentional to keep all crashes around until they're explicitly > >> unlinked from the pstore filesystem from userspace. Especially true > >> for catching chains of kernel crashes, or a failed log collection, > >> etc. Surviving multiple reboots is the expected behavior on Chrome OS > >> too. > > > > Oh, ok. Hence the RFC tag ;-) We can drop this one then. I forgot that > > unlinking was another way to clear the logs. > > In another thread I discovered that the "single prz" ones actually > _are_ zapped at boot. I didn't realize, but it explains why pmsg would > vanish on me sometimes. ;) I always thought I was just doing something > wrong with it. (And I wonder if it's actually a bug that pmsg is > zapped -- console doesn't matter: it's overwritten every boot by > design.) Oh yeah they are. So seems like some are zapped on boot and some aren't then. Hmm, I would think it makes sense not to boot-zap dmesg ever, since that's crash logs someone may want to see after many reboots. But console and pmsg should be since those just "what happened on the last boot". I guess it should be made clear in some structure or something which types are zapped on boot, and which ones aren't. That'll make it clear when adding a new type about that behavior, instead of relying on the assumption that single prz are zapped and multiple ones aren't. Like for ftrace, since the per-cpu configuration was added, it will now be zapped on boot if it is using a per-cpu configuration and not zapped on boot if it isn't right? That would seem a bit inconsistent. thanks! -Joel
Re: [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
On Fri, Oct 26, 2018 at 7:22 PM, Joel Fernandes wrote: > On Fri, Oct 26, 2018 at 07:16:28PM +0100, Kees Cook wrote: >> On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google) >> wrote: >> > This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf. >> > >> > Due to the commit which is being reverted here, it is not possible to >> > know if pstore's messages were from a previous boot, or from very old >> > boots. This creates an awkard situation where its unclear if crash or >> > other logs are from the previous boot or from very old boots. Also >> > typically we dump the pstore buffers after one reboot and are interested >> > in only the previous boot's crash so let us reset the buffer after we >> > save them. >> > >> > Lastly, if we don't zap them, then I think it is possible that part of >> > the buffer will be from this boot and the other parts will be from >> > previous boots. So this revert fixes all of this by calling >> > persistent_ram_zap always. >> >> I like the other patches (comments coming), but not this one: it's >> very intentional to keep all crashes around until they're explicitly >> unlinked from the pstore filesystem from userspace. Especially true >> for catching chains of kernel crashes, or a failed log collection, >> etc. Surviving multiple reboots is the expected behavior on Chrome OS >> too. > > Oh, ok. Hence the RFC tag ;-) We can drop this one then. I forgot that > unlinking was another way to clear the logs. In another thread I discovered that the "single prz" ones actually _are_ zapped at boot. I didn't realize, but it explains why pmsg would vanish on me sometimes. ;) I always thought I was just doing something wrong with it. (And I wonder if it's actually a bug that pmsg is zapped -- console doesn't matter: it's overwritten every boot by design.) -- Kees Cook
Re: [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
On Fri, Oct 26, 2018 at 7:22 PM, Joel Fernandes wrote: > On Fri, Oct 26, 2018 at 07:16:28PM +0100, Kees Cook wrote: >> On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google) >> wrote: >> > This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf. >> > >> > Due to the commit which is being reverted here, it is not possible to >> > know if pstore's messages were from a previous boot, or from very old >> > boots. This creates an awkard situation where its unclear if crash or >> > other logs are from the previous boot or from very old boots. Also >> > typically we dump the pstore buffers after one reboot and are interested >> > in only the previous boot's crash so let us reset the buffer after we >> > save them. >> > >> > Lastly, if we don't zap them, then I think it is possible that part of >> > the buffer will be from this boot and the other parts will be from >> > previous boots. So this revert fixes all of this by calling >> > persistent_ram_zap always. >> >> I like the other patches (comments coming), but not this one: it's >> very intentional to keep all crashes around until they're explicitly >> unlinked from the pstore filesystem from userspace. Especially true >> for catching chains of kernel crashes, or a failed log collection, >> etc. Surviving multiple reboots is the expected behavior on Chrome OS >> too. > > Oh, ok. Hence the RFC tag ;-) We can drop this one then. I forgot that > unlinking was another way to clear the logs. In another thread I discovered that the "single prz" ones actually _are_ zapped at boot. I didn't realize, but it explains why pmsg would vanish on me sometimes. ;) I always thought I was just doing something wrong with it. (And I wonder if it's actually a bug that pmsg is zapped -- console doesn't matter: it's overwritten every boot by design.) -- Kees Cook
Re: [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
On Fri, Oct 26, 2018 at 07:16:28PM +0100, Kees Cook wrote: > On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google) > wrote: > > This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf. > > > > Due to the commit which is being reverted here, it is not possible to > > know if pstore's messages were from a previous boot, or from very old > > boots. This creates an awkard situation where its unclear if crash or > > other logs are from the previous boot or from very old boots. Also > > typically we dump the pstore buffers after one reboot and are interested > > in only the previous boot's crash so let us reset the buffer after we > > save them. > > > > Lastly, if we don't zap them, then I think it is possible that part of > > the buffer will be from this boot and the other parts will be from > > previous boots. So this revert fixes all of this by calling > > persistent_ram_zap always. > > I like the other patches (comments coming), but not this one: it's > very intentional to keep all crashes around until they're explicitly > unlinked from the pstore filesystem from userspace. Especially true > for catching chains of kernel crashes, or a failed log collection, > etc. Surviving multiple reboots is the expected behavior on Chrome OS > too. Oh, ok. Hence the RFC tag ;-) We can drop this one then. I forgot that unlinking was another way to clear the logs. thanks! - Joel
Re: [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
On Fri, Oct 26, 2018 at 07:16:28PM +0100, Kees Cook wrote: > On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google) > wrote: > > This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf. > > > > Due to the commit which is being reverted here, it is not possible to > > know if pstore's messages were from a previous boot, or from very old > > boots. This creates an awkard situation where its unclear if crash or > > other logs are from the previous boot or from very old boots. Also > > typically we dump the pstore buffers after one reboot and are interested > > in only the previous boot's crash so let us reset the buffer after we > > save them. > > > > Lastly, if we don't zap them, then I think it is possible that part of > > the buffer will be from this boot and the other parts will be from > > previous boots. So this revert fixes all of this by calling > > persistent_ram_zap always. > > I like the other patches (comments coming), but not this one: it's > very intentional to keep all crashes around until they're explicitly > unlinked from the pstore filesystem from userspace. Especially true > for catching chains of kernel crashes, or a failed log collection, > etc. Surviving multiple reboots is the expected behavior on Chrome OS > too. Oh, ok. Hence the RFC tag ;-) We can drop this one then. I forgot that unlinking was another way to clear the logs. thanks! - Joel
Re: [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google) wrote: > This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf. > > Due to the commit which is being reverted here, it is not possible to > know if pstore's messages were from a previous boot, or from very old > boots. This creates an awkard situation where its unclear if crash or > other logs are from the previous boot or from very old boots. Also > typically we dump the pstore buffers after one reboot and are interested > in only the previous boot's crash so let us reset the buffer after we > save them. > > Lastly, if we don't zap them, then I think it is possible that part of > the buffer will be from this boot and the other parts will be from > previous boots. So this revert fixes all of this by calling > persistent_ram_zap always. I like the other patches (comments coming), but not this one: it's very intentional to keep all crashes around until they're explicitly unlinked from the pstore filesystem from userspace. Especially true for catching chains of kernel crashes, or a failed log collection, etc. Surviving multiple reboots is the expected behavior on Chrome OS too. -- Kees Cook
Re: [RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google) wrote: > This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf. > > Due to the commit which is being reverted here, it is not possible to > know if pstore's messages were from a previous boot, or from very old > boots. This creates an awkard situation where its unclear if crash or > other logs are from the previous boot or from very old boots. Also > typically we dump the pstore buffers after one reboot and are interested > in only the previous boot's crash so let us reset the buffer after we > save them. > > Lastly, if we don't zap them, then I think it is possible that part of > the buffer will be from this boot and the other parts will be from > previous boots. So this revert fixes all of this by calling > persistent_ram_zap always. I like the other patches (comments coming), but not this one: it's very intentional to keep all crashes around until they're explicitly unlinked from the pstore filesystem from userspace. Especially true for catching chains of kernel crashes, or a failed log collection, etc. Surviving multiple reboots is the expected behavior on Chrome OS too. -- Kees Cook
[RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf.
Due to the commit which is being reverted here, it is not possible to
know if pstore's messages were from a previous boot, or from very old
boots. This creates an awkard situation where its unclear if crash or
other logs are from the previous boot or from very old boots. Also
typically we dump the pstore buffers after one reboot and are interested
in only the previous boot's crash so let us reset the buffer after we
save them.
Lastly, if we don't zap them, then I think it is possible that part of
the buffer will be from this boot and the other parts will be from
previous boots. So this revert fixes all of this by calling
persistent_ram_zap always.
Signed-off-by: Joel Fernandes (Google)
---
fs/pstore/ram_core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 1299aa3ea734..67d74dd97da1 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -504,7 +504,6 @@ static int persistent_ram_post_init(struct
persistent_ram_zone *prz, u32 sig,
pr_debug("found existing buffer, size %zu, start %zu\n",
buffer_size(prz), buffer_start(prz));
persistent_ram_save_old(prz);
- return 0;
}
} else {
pr_debug("no valid data in buffer (sig = 0x%08x)\n",
--
2.19.1.568.g152ad8e336-goog
[RFC 6/6] Revert "pstore/ram_core: Do not reset restored zone's position and size"
This reverts commit 25b63da64708212985c06c7f8b089d356efdd9cf.
Due to the commit which is being reverted here, it is not possible to
know if pstore's messages were from a previous boot, or from very old
boots. This creates an awkard situation where its unclear if crash or
other logs are from the previous boot or from very old boots. Also
typically we dump the pstore buffers after one reboot and are interested
in only the previous boot's crash so let us reset the buffer after we
save them.
Lastly, if we don't zap them, then I think it is possible that part of
the buffer will be from this boot and the other parts will be from
previous boots. So this revert fixes all of this by calling
persistent_ram_zap always.
Signed-off-by: Joel Fernandes (Google)
---
fs/pstore/ram_core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 1299aa3ea734..67d74dd97da1 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -504,7 +504,6 @@ static int persistent_ram_post_init(struct
persistent_ram_zone *prz, u32 sig,
pr_debug("found existing buffer, size %zu, start %zu\n",
buffer_size(prz), buffer_start(prz));
persistent_ram_save_old(prz);
- return 0;
}
} else {
pr_debug("no valid data in buffer (sig = 0x%08x)\n",
--
2.19.1.568.g152ad8e336-goog
