On Mon, 2019-02-25 at 15:43 -0700, Vishal Verma wrote:
> The Linux BTT implementation assumes that log entries will never have
> the 'zero' flag set, and indeed it never sets that flag for log entries
> itself.
>
> However, the UEFI spec is ambiguous on the exact format of the LBA field
> of a log entry, specifically as to whether it should include the
> additional flag bits or not. While a zero bit doesn't make sense in the
> context of a log entry, other BTT implementations might still have it set.
>
> If an implementation does happen to have it set, we would happily read
> it in as the next block to write to for writes. Since a high bit is set,
> it pushes the block number out of the range of an 'arena', and we fail
> such a write with an EIO.
>
> Follow the robustness principle, and tolerate such implementations by
> stripping out the zero flag when populating the free list during
> initialization. Additionally, use the same stripped out entries for
> detection of incomplete writes and map restoration that happens at this
> stage.
>
> Cc: Dan Williams <[email protected]>
> Reported-by: Dexuan Cui <[email protected]>
> Reported-by: Pedro d'Aquino Filocre F S Barbuda <[email protected]>
> Tested-by: Dexuan Cui <[email protected]>
> Signed-off-by: Vishal Verma <[email protected]>
> ---
Forgot to include a change description:
v1 -> v2:
- Add a patch to remove unused code getting the old log entry
- Also use the stripped out versions of the log entries when testing for
incomplete writes.
> drivers/nvdimm/btt.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index cd4fa87ea48c..294c48e45e74 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -542,8 +542,8 @@ static int arena_clear_freelist_error(struct arena_info
> *arena, u32 lane)
> static int btt_freelist_init(struct arena_info *arena)
> {
> int new, ret;
> - u32 i, map_entry;
> struct log_entry log_new;
> + u32 i, map_entry, log_oldmap, log_newmap;
>
> arena->freelist = kcalloc(arena->nfree, sizeof(struct free_entry),
> GFP_KERNEL);
> @@ -555,16 +555,21 @@ static int btt_freelist_init(struct arena_info *arena)
> if (new < 0)
> return new;
>
> + /* old and new map entries with any flags stripped out */
> + log_oldmap = ent_lba(le32_to_cpu(log_new.old_map));
> + log_newmap = ent_lba(le32_to_cpu(log_new.new_map));
> +
> /* sub points to the next one to be overwritten */
> arena->freelist[i].sub = 1 - new;
> arena->freelist[i].seq = nd_inc_seq(le32_to_cpu(log_new.seq));
> - arena->freelist[i].block = le32_to_cpu(log_new.old_map);
> + arena->freelist[i].block = log_oldmap;
>
> /*
> * FIXME: if error clearing fails during init, we want to make
> * the BTT read-only
> */
> if (ent_e_flag(log_new.old_map)) {
> + set_e_flag(arena->freelist[i].block);
> ret = arena_clear_freelist_error(arena, i);
> if (ret)
> dev_err_ratelimited(to_dev(arena),
> @@ -572,7 +577,7 @@ static int btt_freelist_init(struct arena_info *arena)
> }
>
> /* This implies a newly created or untouched flog entry */
> - if (log_new.old_map == log_new.new_map)
> + if (log_oldmap == log_newmap)
> continue;
>
> /* Check if map recovery is needed */
> @@ -580,8 +585,15 @@ static int btt_freelist_init(struct arena_info *arena)
> NULL, NULL, 0);
> if (ret)
> return ret;
> - if ((le32_to_cpu(log_new.new_map) != map_entry) &&
> - (le32_to_cpu(log_new.old_map) == map_entry)) {
> +
> + /*
> + * The map_entry from btt_read_map is stripped of any flag bits,
> + * so use the stripped out versions from the log as well for
> + * testing whether recovery is needed. For restoration, use the
> + * 'raw' version of the log entries as that captured what we
> + * were going to write originally.
> + */
> + if ((log_newmap != map_entry) && (log_oldmap == map_entry)) {
> /*
> * Last transaction wrote the flog, but wasn't able
> * to complete the map write. So fix up the map.
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
