Re: [PATCH] jffs2: make the overwritten xattr invisible after remount

[Hou Tao]

ping ?

On 2018/12/9 14:21, Hou Tao wrote:
> For xattr modification, we do not write a new jffs2_raw_xref with
> delete marker into flash, so if a xattr is modified then removed,
> and the old xref & xdatum are not erased by GC, after reboot or
> remount, the new xattr xref will be dead but the old xattr xref
> will be alive, and we will get the overwritten xattr instead of
> non-existent error when reading the removed xattr.
> 
> Fix it by writing the deletion mark for xattr overwrite.
> 
> Fixes: 8a13695cbe4e ("[JFFS2][XATTR] rid unnecessary writing of delete marker.")
> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
> ---
>  fs/jffs2/xattr.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/jffs2/xattr.c b/fs/jffs2/xattr.c
> index da3e18503c65..b2d6072f34af 100644
> --- a/fs/jffs2/xattr.c
> +++ b/fs/jffs2/xattr.c
> @@ -573,6 +573,15 @@ static struct jffs2_xattr_ref *create_xattr_ref(struct jffs2_sb_info *c, struct
>  	return ref; /* success */
>  }
>  
> +static void move_xattr_ref_to_dead_list(struct jffs2_sb_info *c,
> +		struct jffs2_xattr_ref *ref)
> +{
> +	spin_lock(&c->erase_completion_lock);
> +	ref->next = c->xref_dead_list;
> +	c->xref_dead_list = ref;
> +	spin_unlock(&c->erase_completion_lock);
> +}
> +
>  static void delete_xattr_ref(struct jffs2_sb_info *c, struct jffs2_xattr_ref *ref)
>  {
>  	/* must be called under down_write(xattr_sem) */
> @@ -582,10 +591,7 @@ static void delete_xattr_ref(struct jffs2_sb_info *c, struct jffs2_xattr_ref *re
>  	ref->xseqno |= XREF_DELETE_MARKER;
>  	ref->ino = ref->ic->ino;
>  	ref->xid = ref->xd->xid;
> -	spin_lock(&c->erase_completion_lock);
> -	ref->next = c->xref_dead_list;
> -	c->xref_dead_list = ref;
> -	spin_unlock(&c->erase_completion_lock);
> +	move_xattr_ref_to_dead_list(c, ref);
>  
>  	dbg_xattr("xref(ino=%u, xid=%u, xseqno=%u) was removed.\n",
>  		  ref->ino, ref->xid, ref->xseqno);
> @@ -1090,6 +1096,40 @@ int do_jffs2_getxattr(struct inode *inode, int xprefix, const char *xname,
>  	return rc;
>  }
>  
> +static void do_jffs2_delete_xattr_ref(struct jffs2_sb_info *c,
> +		struct jffs2_xattr_ref *ref)
> +{
> +	uint32_t request, length;
> +	int err;
> +	struct jffs2_xattr_datum *xd;
> +
> +	request = PAD(sizeof(struct jffs2_raw_xref));
> +	err = jffs2_reserve_space(c, request, &length,
> +			ALLOC_NORMAL, JFFS2_SUMMARY_XREF_SIZE);
> +	down_write(&c->xattr_sem);
> +	if (err) {
> +		JFFS2_WARNING("jffs2_reserve_space()=%d, request=%u\n",
> +				err, request);
> +		delete_xattr_ref(c, ref);
> +		up_write(&c->xattr_sem);
> +		return;
> +	}
> +
> +	xd = ref->xd;
> +	ref->ino = ref->ic->ino;
> +	ref->xid = xd->xid;
> +	ref->xseqno |= XREF_DELETE_MARKER;
> +	save_xattr_ref(c, ref);
> +
> +	move_xattr_ref_to_dead_list(c, ref);
> +	dbg_xattr("xref(ino=%u, xid=%u, xseqno=%u) was removed.\n",
> +		  ref->ino, ref->xid, ref->xseqno);
> +	unrefer_xattr_datum(c, xd);
> +
> +	up_write(&c->xattr_sem);
> +	jffs2_complete_reservation(c);
> +}
> +
>  int do_jffs2_setxattr(struct inode *inode, int xprefix, const char *xname,
>  		      const char *buffer, size_t size, int flags)
>  {
> @@ -1097,7 +1137,7 @@ int do_jffs2_setxattr(struct inode *inode, int xprefix, const char *xname,
>  	struct jffs2_sb_info *c = JFFS2_SB_INFO(inode->i_sb);
>  	struct jffs2_inode_cache *ic = f->inocache;
>  	struct jffs2_xattr_datum *xd;
> -	struct jffs2_xattr_ref *ref, *newref, **pref;
> +	struct jffs2_xattr_ref *ref, *newref, *oldref, **pref;
>  	uint32_t length, request;
>  	int rc;
>  
> @@ -1113,6 +1153,7 @@ int do_jffs2_setxattr(struct inode *inode, int xprefix, const char *xname,
>  		return rc;
>  	}
>  
> +	oldref = NULL;
>  	/* Find existing xattr */
>  	down_write(&c->xattr_sem);
>   retry:
> @@ -1196,11 +1237,13 @@ int do_jffs2_setxattr(struct inode *inode, int xprefix, const char *xname,
>  		rc = PTR_ERR(newref);
>  		unrefer_xattr_datum(c, xd);
>  	} else if (ref) {
> -		delete_xattr_ref(c, ref);
> +		oldref = ref;
>  	}
>   out:
>  	up_write(&c->xattr_sem);
>  	jffs2_complete_reservation(c);
> +	if (oldref)
> +		do_jffs2_delete_xattr_ref(c, oldref);
>  	return rc;
>  }
>  
>