Re: [PATCH 1/2] UBIFS: fix use of freed ubifs_orphan objects

[Adrian Hunter <adrian.hunter@intel.com>]

On 28/01/13 03:28, Adam Thomas wrote:
> The last orphan in the cnext list has its cnext set to NULL. Because
> of that, ubifs_delete_orphan assumes that it is not on the cnext list
> and frees it immediately instead of adding it to the dnext list. The
> freed orphan is later modified by write_orph_node.

Very true!

> 
> This can cause various inconsistencies including directory entries
> that cannot be removed and this error:
> 
> UBIFS error (pid 20685): layout_cnodes: LPT out of space at LEB 14:129009 needing 17, done_ltab 1, done_lsave 1
> 
> This is a regression introduced by
> "7074e5eb UBIFS: remove invalid reference to list iterator variable".
> 
> This change adds an explicit flag to ubifs_orphan indicating whether
> it is pending commit.

OK

Needs an signed-off line.

> ---
>  fs/ubifs/orphan.c |    6 +++++-
>  fs/ubifs/ubifs.h  |    4 +++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ubifs/orphan.c b/fs/ubifs/orphan.c
> index 769701c..8433f53 100644
> --- a/fs/ubifs/orphan.c
> +++ b/fs/ubifs/orphan.c
> @@ -132,7 +132,7 @@ void ubifs_delete_orphan(struct ubifs_info *c, ino_t inum)
>  					(unsigned long)inum);
>  				return;
>  			}
> -			if (o->cnext) {
> +			if (o->cmt) {
>  				o->dnext = c->orph_dnext;
>  				c->orph_dnext = o;
>  				spin_unlock(&c->orphan_lock);
> @@ -172,7 +172,9 @@ int ubifs_orphan_start_commit(struct ubifs_info *c)
>  	last = &c->orph_cnext;
>  	list_for_each_entry(orphan, &c->orph_new, new_list) {
>  		ubifs_assert(orphan->new);
> +		ubifs_assert(!orphan->cmt);
>  		orphan->new = 0;
> +		orphan->cmt = 1;
>  		*last = orphan;
>  		last = &orphan->cnext;
>  	}
> @@ -299,6 +301,7 @@ static int write_orph_node(struct ubifs_info *c, int atomic)
>  	cnext = c->orph_cnext;
>  	for (i = 0; i < cnt; i++) {
>  		orphan = cnext;
> +		ubifs_assert(orphan->cmt);
>  		orph->inos[i] = cpu_to_le64(orphan->inum);
>  		cnext = orphan->cnext;
>  		orphan->cnext = NULL;
> @@ -378,6 +381,7 @@ static int consolidate(struct ubifs_info *c)
>  		list_for_each_entry(orphan, &c->orph_list, list) {
>  			if (orphan->new)
>  				continue;
> +			orphan->cmt = 1;
>  			*last = orphan;
>  			last = &orphan->cnext;
>  			cnt += 1;
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index d133c27..c16fff7 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -904,6 +904,7 @@ struct ubifs_budget_req {
>   * @dnext: next orphan to delete
>   * @inum: inode number
>   * @new: %1 => added since the last commit, otherwise %0
> + * @cmt: %1 => commit pending, otherwise %0

You need to set cmt to zero in write_orph_node().

>   */
>  struct ubifs_orphan {
>  	struct rb_node rb;
> @@ -912,7 +913,8 @@ struct ubifs_orphan {
>  	struct ubifs_orphan *cnext;
>  	struct ubifs_orphan *dnext;
>  	ino_t inum;
> -	int new;
> +	unsigned new:1;
> +	unsigned cmt:1;
>  };
>  
>  /**
>