Re: [Cluster-devel] [PATCH 2/2] gfs2: Add trusted xattr support

[Andrew Price]

On 05/02/2021 19:04, Bob Peterson wrote:

Hi,

Comments below.

- Original Message -

From: Andreas Gruenbacher 

Add support for an additional filesystem version (sb_fs_format = 1802).
When a filesystem with the new version is mounted, the filesystem
supports "trusted.*" xattrs.

In addition, version 1802 filesystems implement a form of forward
compatibility for xattrs: when xattrs with an unknown prefix (ea_type)
are found on a version 1802 filesystem, those attributes are not shown
by listxattr, and they are not accessible by getxattr, setxattr, or
removexattr.

This mechanism might turn out to be what we need in the future, but if
not, we can always bump the filesystem version and break compatibility
instead.

Signed-off-by: Andrew Price 


If this is from Andreas, it should have his Signed-off-by:


Yes, it wasn't in the original patch (posted on bugzilla). With Andreas' 
permission I'll add it to the next version.





---
  fs/gfs2/ops_fstype.c | 14 +-
  fs/gfs2/super.h  |  4 ++-
  fs/gfs2/xattr.c  | 48 +---
  include/uapi/linux/gfs2_ondisk.h |  5 ++--
  4 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 52fe78378faa..64ad19bb978c 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -489,6 +489,19 @@ static int init_sb(struct gfs2_sbd *sdp, int silent)
goto out;
}
  
+	switch(sdp->sd_sb.sb_fs_format) {

+   case GFS2_FS_FORMAT_MAX:
+   sb->s_xattr = gfs2_xattr_handlers_max;
+   break;
+
+   case GFS2_FS_FORMAT_MIN:
+   sb->s_xattr = gfs2_xattr_handlers_min;
+   break;
+
+   default:
+   BUG();
+   }
+
/* Set up the buffer cache and SB for real */
if (sdp->sd_sb.sb_bsize < bdev_logical_block_size(sb->s_bdev)) {
ret = -EINVAL;
@@ -1109,7 +1122,6 @@ static int gfs2_fill_super(struct super_block *sb,
struct fs_context *fc)
sb->s_op = _super_ops;
sb->s_d_op = _dops;
sb->s_export_op = _export_ops;
-   sb->s_xattr = gfs2_xattr_handlers;
sb->s_qcop = _quotactl_ops;
sb->s_quota_types = QTYPE_MASK_USR | QTYPE_MASK_GRP;
sb_dqopt(sb)->flags |= DQUOT_QUOTA_SYS_FILE;
diff --git a/fs/gfs2/super.h b/fs/gfs2/super.h
index 977079693bdc..08e502dec7ec 100644
--- a/fs/gfs2/super.h
+++ b/fs/gfs2/super.h
@@ -58,7 +58,9 @@ extern struct file_system_type gfs2meta_fs_type;
  extern const struct export_operations gfs2_export_ops;
  extern const struct super_operations gfs2_super_ops;
  extern const struct dentry_operations gfs2_dops;
-extern const struct xattr_handler *gfs2_xattr_handlers[];
+
+extern const struct xattr_handler *gfs2_xattr_handlers_max[];
+extern const struct xattr_handler **gfs2_xattr_handlers_min;
  
  #endif /* __SUPER_DOT_H__ */
  
diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c

index 9d7667bc4292..a860a144f3d4 100644
--- a/fs/gfs2/xattr.c
+++ b/fs/gfs2/xattr.c
@@ -70,6 +70,20 @@ static int ea_check_size(struct gfs2_sbd *sdp, unsigned
int nsize, size_t dsize)
return 0;
  }
  
+bool gfs2_eatype_valid(struct gfs2_sbd *sdp, u8 type)


This should be "static bool".


Ok, I'll update that.




+{
+   switch(sdp->sd_sb.sb_fs_format) {
+   case GFS2_FS_FORMAT_MAX:
+   return true;
+
+   case GFS2_FS_FORMAT_MIN:
+   return type <= GFS2_EATYPE_SECURITY;
+
+   default:
+   return false;
+   }
+}
+
  typedef int (*ea_call_t) (struct gfs2_inode *ip, struct buffer_head *bh,
  struct gfs2_ea_header *ea,
  struct gfs2_ea_header *prev, void *private);
@@ -77,6 +91,7 @@ typedef int (*ea_call_t) (struct gfs2_inode *ip, struct
buffer_head *bh,
  static int ea_foreach_i(struct gfs2_inode *ip, struct buffer_head *bh,
ea_call_t ea_call, void *data)
  {
+   struct gfs2_sbd *sdp = GFS2_SB(>i_inode);
struct gfs2_ea_header *ea, *prev = NULL;
int error = 0;
  
@@ -89,9 +104,8 @@ static int ea_foreach_i(struct gfs2_inode *ip, struct

buffer_head *bh,
if (!(bh->b_data <= (char *)ea && (char *)GFS2_EA2NEXT(ea) <=
  bh->b_data + bh->b_size))
goto fail;
-   if (!GFS2_EATYPE_VALID(ea->ea_type))
+   if (!gfs2_eatype_valid(sdp, ea->ea_type))
goto fail;
-
error = ea_call(ip, bh, ea, prev, data);
if (error)
return error;
@@ -344,6 +358,7 @@ static int ea_list_i(struct gfs2_inode *ip, struct
buffer_head *bh,
 struct gfs2_ea_header *ea, struct gfs2_ea_header *prev,
 void *private)
  {
+   struct gfs2_sbd *sdp = GFS2_SB(>i_inode);
struct ea_list *ei = private;
struct gfs2_ea_request *er = ei->ei_er;

Re: [Cluster-devel] [gfs2 PATCH] gfs2: Don't skip dlm unlock if glock has an lvb

[Steven Whitehouse]

Hi,

Longer term we should review whether this is really the correct fix. It 
seems a bit strange that we have to do something different according to 
whether there is an LVB or not. We are gradually increasing LVB use over 
time too. So should we fix the DLM so that either it can cope with locks 
with LVBs at lockspace shutdown time, or should we simply send an unlock 
for all DLM locks anyway? That would seem to make more sense than having 
two different systems depending on LVB existence, or otherwise,


Steve.

On 05/02/2021 18:50, Bob Peterson wrote:

Patch fb6791d100d1bba20b5cdbc4912e1f7086ec60f8 was designed to allow
gfs2 to unmount quicker by skipping the step where it tells dlm to
unlock glocks in EX with lvbs. This was done because when gfs2 unmounts
a file system, it destroys the dlm lockspace shortly after it destroys
the glocks so it doesn't need to unlock them all: the unlock is implied
when the lockspace is destroyed by dlm.

However, that patch introduced a use-after-free in dlm: as part of its
normal dlm_recoverd process, it can call ls_recovery to recover dead
locks. In so doing, it can call recover_rsbs which calls recover_lvb for
any mastered rsbs. Func recover_lvb runs through the list of lkbs queued
to the given rsb (if the glock is cached but unlocked, it will still be
queued to the lkb, but in NL--Unlocked--mode) and if it has an lvb,
copies it to the rsb, thus trying to preserve the lkb. However, when
gfs2 skips the dlm unlock step, it frees the glock and its lvb, which
means dlm's function recover_lvb references the now freed lvb pointer,
copying the freed lvb memory to the rsb.

This patch changes the check in gdlm_put_lock so that it calls dlm_unlock
for all glocks that contain an lvb pointer.

Signed-off-by: Bob Peterson 
Fixes: fb6791d100d1 "GFS2: skip dlm_unlock calls in unmount"
---
  fs/gfs2/lock_dlm.c | 8 ++--
  1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
index 9f2b5609f225..153272f82984 100644
--- a/fs/gfs2/lock_dlm.c
+++ b/fs/gfs2/lock_dlm.c
@@ -284,7 +284,6 @@ static void gdlm_put_lock(struct gfs2_glock *gl)
  {
struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
struct lm_lockstruct *ls = >sd_lockstruct;
-   int lvb_needs_unlock = 0;
int error;
  
  	if (gl->gl_lksb.sb_lkid == 0) {

@@ -297,13 +296,10 @@ static void gdlm_put_lock(struct gfs2_glock *gl)
gfs2_sbstats_inc(gl, GFS2_LKS_DCOUNT);
gfs2_update_request_times(gl);
  
-	/* don't want to skip dlm_unlock writing the lvb when lock is ex */

-
-   if (gl->gl_lksb.sb_lvbptr && (gl->gl_state == LM_ST_EXCLUSIVE))
-   lvb_needs_unlock = 1;
+   /* don't want to skip dlm_unlock writing the lvb when lock has one */
  
  	if (test_bit(SDF_SKIP_DLM_UNLOCK, >sd_flags) &&

-   !lvb_needs_unlock) {
+   !gl->gl_lksb.sb_lvbptr) {
gfs2_glock_free(gl);
return;
}