Re: [RFC 5/6] pstore: donot treat empty buffers as valid
On Fri, Oct 26, 2018 at 08:39:13PM +0100, Kees Cook wrote: > On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google) > wrote: > > pstore currently calls persistent_ram_save_old even if a buffer is > > empty. While this appears to work, it is simply not the right thing to > > do and could lead to bugs so lets avoid that. It also prevent misleading > > prints in the logs which claim the buffer is valid. > > I need to be better convinced that a present zero length record is the > same as a non-present record. This seems true, but there is > potentially still metadata available from a backend. What were the > misleading prints in logs? I got something like: found existing buffer, size 0, start 0 When I was expecting: no valid data in buffer (sig = ...) The other thing is a call to persistent_ram_zap is also prevented on the buffer, which prevent zero-initialize prz->ecc_info.par. Since we are dropping patch 6/6, the zap will not happen. But I'm not super familiar with the ecc bits of this code to say that if that's an issue. About the present zero-length record, I would argue that it should not be "present" at all. When the system first boots, the record is not present but the signatures are initialized, then on reboots because the signatures were initialized, the buffer appears as valid even if it was unused. So for dmesg, all the max_dump_cnt number of buffers would appear as if they are valid which is a bit strange because there was no crash at all so it should be in the same state on reboot, as if there was no crash. That could be a matter of perspective though so I leave it you how you prefer to do it :) thanks, - Joel
Re: [RFC 5/6] pstore: donot treat empty buffers as valid
On Fri, Oct 26, 2018 at 08:39:13PM +0100, Kees Cook wrote: > On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google) > wrote: > > pstore currently calls persistent_ram_save_old even if a buffer is > > empty. While this appears to work, it is simply not the right thing to > > do and could lead to bugs so lets avoid that. It also prevent misleading > > prints in the logs which claim the buffer is valid. > > I need to be better convinced that a present zero length record is the > same as a non-present record. This seems true, but there is > potentially still metadata available from a backend. What were the > misleading prints in logs? I got something like: found existing buffer, size 0, start 0 When I was expecting: no valid data in buffer (sig = ...) The other thing is a call to persistent_ram_zap is also prevented on the buffer, which prevent zero-initialize prz->ecc_info.par. Since we are dropping patch 6/6, the zap will not happen. But I'm not super familiar with the ecc bits of this code to say that if that's an issue. About the present zero-length record, I would argue that it should not be "present" at all. When the system first boots, the record is not present but the signatures are initialized, then on reboots because the signatures were initialized, the buffer appears as valid even if it was unused. So for dmesg, all the max_dump_cnt number of buffers would appear as if they are valid which is a bit strange because there was no crash at all so it should be in the same state on reboot, as if there was no crash. That could be a matter of perspective though so I leave it you how you prefer to do it :) thanks, - Joel
Re: [RFC 5/6] pstore: donot treat empty buffers as valid
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
wrote:
> pstore currently calls persistent_ram_save_old even if a buffer is
> empty. While this appears to work, it is simply not the right thing to
> do and could lead to bugs so lets avoid that. It also prevent misleading
> prints in the logs which claim the buffer is valid.
I need to be better convinced that a present zero length record is the
same as a non-present record. This seems true, but there is
potentially still metadata available from a backend. What were the
misleading prints in logs?
-Kees
>
> Signed-off-by: Joel Fernandes (Google)
> ---
> fs/pstore/ram_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index 0792595ebcfb..1299aa3ea734 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -495,7 +495,7 @@ static int persistent_ram_post_init(struct
> persistent_ram_zone *prz, u32 sig,
>
> sig ^= PERSISTENT_RAM_SIG;
>
> - if (prz->buffer->sig == sig) {
> + if (prz->buffer->sig == sig && buffer_size(prz)) {
> if (buffer_size(prz) > prz->buffer_size ||
> buffer_start(prz) > buffer_size(prz))
> pr_info("found existing invalid buffer, size %zu,
> start %zu\n",
> --
> 2.19.1.568.g152ad8e336-goog
>
--
Kees Cook
Re: [RFC 5/6] pstore: donot treat empty buffers as valid
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
wrote:
> pstore currently calls persistent_ram_save_old even if a buffer is
> empty. While this appears to work, it is simply not the right thing to
> do and could lead to bugs so lets avoid that. It also prevent misleading
> prints in the logs which claim the buffer is valid.
I need to be better convinced that a present zero length record is the
same as a non-present record. This seems true, but there is
potentially still metadata available from a backend. What were the
misleading prints in logs?
-Kees
>
> Signed-off-by: Joel Fernandes (Google)
> ---
> fs/pstore/ram_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index 0792595ebcfb..1299aa3ea734 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -495,7 +495,7 @@ static int persistent_ram_post_init(struct
> persistent_ram_zone *prz, u32 sig,
>
> sig ^= PERSISTENT_RAM_SIG;
>
> - if (prz->buffer->sig == sig) {
> + if (prz->buffer->sig == sig && buffer_size(prz)) {
> if (buffer_size(prz) > prz->buffer_size ||
> buffer_start(prz) > buffer_size(prz))
> pr_info("found existing invalid buffer, size %zu,
> start %zu\n",
> --
> 2.19.1.568.g152ad8e336-goog
>
--
Kees Cook
[RFC 5/6] pstore: donot treat empty buffers as valid
pstore currently calls persistent_ram_save_old even if a buffer is
empty. While this appears to work, it is simply not the right thing to
do and could lead to bugs so lets avoid that. It also prevent misleading
prints in the logs which claim the buffer is valid.
Signed-off-by: Joel Fernandes (Google)
---
fs/pstore/ram_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 0792595ebcfb..1299aa3ea734 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -495,7 +495,7 @@ static int persistent_ram_post_init(struct
persistent_ram_zone *prz, u32 sig,
sig ^= PERSISTENT_RAM_SIG;
- if (prz->buffer->sig == sig) {
+ if (prz->buffer->sig == sig && buffer_size(prz)) {
if (buffer_size(prz) > prz->buffer_size ||
buffer_start(prz) > buffer_size(prz))
pr_info("found existing invalid buffer, size %zu, start
%zu\n",
--
2.19.1.568.g152ad8e336-goog
[RFC 5/6] pstore: donot treat empty buffers as valid
pstore currently calls persistent_ram_save_old even if a buffer is
empty. While this appears to work, it is simply not the right thing to
do and could lead to bugs so lets avoid that. It also prevent misleading
prints in the logs which claim the buffer is valid.
Signed-off-by: Joel Fernandes (Google)
---
fs/pstore/ram_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 0792595ebcfb..1299aa3ea734 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -495,7 +495,7 @@ static int persistent_ram_post_init(struct
persistent_ram_zone *prz, u32 sig,
sig ^= PERSISTENT_RAM_SIG;
- if (prz->buffer->sig == sig) {
+ if (prz->buffer->sig == sig && buffer_size(prz)) {
if (buffer_size(prz) > prz->buffer_size ||
buffer_start(prz) > buffer_size(prz))
pr_info("found existing invalid buffer, size %zu, start
%zu\n",
--
2.19.1.568.g152ad8e336-goog
