* [Cluster-devel] [PATCH] gfs2: Null-check gl in gfs2_rgrp_dump()
@ 2020-10-07 11:30 Andrew Price
2020-10-14 14:30 ` Andreas Gruenbacher
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Price @ 2020-10-07 11:30 UTC (permalink / raw)
To: cluster-devel.redhat.com
When an rindex entry is found to be corrupt, compute_bitstructs() calls
gfs2_consist_rgrpd() which calls gfs2_rgrp_dump() like this:
gfs2_rgrp_dump(NULL, rgd->rd_gl, fs_id_buf);
gfs2_rgrp_dump then dereferences the gl without checking it and we get
BUG: KASAN: null-ptr-deref in gfs2_rgrp_dump+0x28/0x280
because there's no rgrp glock involved while reading the rindex on mount.
Add a NULL check for gl in gfs2_rgrp_dump() to allow the withdraw to
continue cleanly.
Reported-by: syzbot+43fa87986bdd31df9de6 at syzkaller.appspotmail.com
Signed-off-by: Andrew Price <anprice@redhat.com>
---
fs/gfs2/rgrp.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 074f228ea839..ff0cabd819cc 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -2217,12 +2217,13 @@ static void rgblk_free(struct gfs2_sbd *sdp, struct gfs2_rgrpd *rgd,
void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_glock *gl,
const char *fs_id_buf)
{
- struct gfs2_rgrpd *rgd = gl->gl_object;
+ struct gfs2_rgrpd *rgd;
struct gfs2_blkreserv *trs;
const struct rb_node *n;
- if (rgd == NULL)
+ if (gl == NULL || gl->gl_object == NULL)
return;
+ rgd = gl->gl_object;
gfs2_print_dbg(seq, "%s R: n:%llu f:%02x b:%u/%u i:%u r:%u e:%u\n",
fs_id_buf,
(unsigned long long)rgd->rd_addr, rgd->rd_flags,
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [Cluster-devel] [PATCH] gfs2: Null-check gl in gfs2_rgrp_dump()
2020-10-07 11:30 [Cluster-devel] [PATCH] gfs2: Null-check gl in gfs2_rgrp_dump() Andrew Price
@ 2020-10-14 14:30 ` Andreas Gruenbacher
2020-10-14 15:13 ` Andrew Price
0 siblings, 1 reply; 3+ messages in thread
From: Andreas Gruenbacher @ 2020-10-14 14:30 UTC (permalink / raw)
To: cluster-devel.redhat.com
Hi Andy,
> When an rindex entry is found to be corrupt, compute_bitstructs() calls
> gfs2_consist_rgrpd() which calls gfs2_rgrp_dump() like this:
>
> gfs2_rgrp_dump(NULL, rgd->rd_gl, fs_id_buf);
>
> gfs2_rgrp_dump then dereferences the gl without checking it and we get
>
> BUG: KASAN: null-ptr-deref in gfs2_rgrp_dump+0x28/0x280
>
> because there's no rgrp glock involved while reading the rindex on mount.
>
> Add a NULL check for gl in gfs2_rgrp_dump() to allow the withdraw to
> continue cleanly.
thanks for looking into this. Shouldn't gfs2_rgrp_dump just take an
rgrp argument as below though?
Thanks,
Andreas
---
fs/gfs2/glops.c | 11 ++++++++++-
fs/gfs2/rgrp.c | 9 +++------
fs/gfs2/rgrp.h | 2 +-
fs/gfs2/util.c | 2 +-
4 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index de1d5f1d9ff8..c2c90747d79b 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -227,6 +227,15 @@ static void rgrp_go_inval(struct gfs2_glock *gl, int flags)
rgd->rd_flags &= ~GFS2_RDF_UPTODATE;
}
+static void gfs2_rgrp_go_dump(struct seq_file *seq, struct gfs2_glock *gl,
+ const char *fs_id_buf)
+{
+ struct gfs2_rgrpd *rgd = gfs2_glock2rgrp(gl);
+
+ if (rgd)
+ gfs2_rgrp_dump(seq, rgd, fs_id_buf);
+}
+
static struct gfs2_inode *gfs2_glock2inode(struct gfs2_glock *gl)
{
struct gfs2_inode *ip;
@@ -712,7 +721,7 @@ const struct gfs2_glock_operations gfs2_rgrp_glops = {
.go_sync = rgrp_go_sync,
.go_inval = rgrp_go_inval,
.go_lock = gfs2_rgrp_go_lock,
- .go_dump = gfs2_rgrp_dump,
+ .go_dump = gfs2_rgrp_go_dump,
.go_type = LM_TYPE_RGRP,
.go_flags = GLOF_LVB,
};
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 074f228ea839..1bba5a9d45fa 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -2209,20 +2209,17 @@ static void rgblk_free(struct gfs2_sbd *sdp, struct gfs2_rgrpd *rgd,
/**
* gfs2_rgrp_dump - print out an rgrp
* @seq: The iterator
- * @gl: The glock in question
+ * @rgd: The rgrp in question
* @fs_id_buf: pointer to file system id (if requested)
*
*/
-void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_glock *gl,
+void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_rgrpd *rgd,
const char *fs_id_buf)
{
- struct gfs2_rgrpd *rgd = gl->gl_object;
struct gfs2_blkreserv *trs;
const struct rb_node *n;
- if (rgd == NULL)
- return;
gfs2_print_dbg(seq, "%s R: n:%llu f:%02x b:%u/%u i:%u r:%u e:%u\n",
fs_id_buf,
(unsigned long long)rgd->rd_addr, rgd->rd_flags,
@@ -2253,7 +2250,7 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
(unsigned long long)rgd->rd_addr);
fs_warn(sdp, "umount on all nodes and run fsck.gfs2 to fix the error\n");
sprintf(fs_id_buf, "fsid=%s: ", sdp->sd_fsname);
- gfs2_rgrp_dump(NULL, rgd->rd_gl, fs_id_buf);
+ gfs2_rgrp_dump(NULL, rgd, fs_id_buf);
rgd->rd_flags |= GFS2_RDF_ERROR;
}
diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
index a1d7e14fc55b..9a587ada51ed 100644
--- a/fs/gfs2/rgrp.h
+++ b/fs/gfs2/rgrp.h
@@ -67,7 +67,7 @@ extern void gfs2_rlist_add(struct gfs2_inode *ip, struct gfs2_rgrp_list *rlist,
extern void gfs2_rlist_alloc(struct gfs2_rgrp_list *rlist);
extern void gfs2_rlist_free(struct gfs2_rgrp_list *rlist);
extern u64 gfs2_ri_total(struct gfs2_sbd *sdp);
-extern void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_glock *gl,
+extern void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_rgrpd *rgd,
const char *fs_id_buf);
extern int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
struct buffer_head *bh,
diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
index 1cd0328cae20..0fba3bf64189 100644
--- a/fs/gfs2/util.c
+++ b/fs/gfs2/util.c
@@ -419,7 +419,7 @@ void gfs2_consist_rgrpd_i(struct gfs2_rgrpd *rgd,
char fs_id_buf[sizeof(sdp->sd_fsname) + 7];
sprintf(fs_id_buf, "fsid=%s: ", sdp->sd_fsname);
- gfs2_rgrp_dump(NULL, rgd->rd_gl, fs_id_buf);
+ gfs2_rgrp_dump(NULL, rgd, fs_id_buf);
gfs2_lm(sdp,
"fatal: filesystem consistency error\n"
" RG = %llu\n"
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [Cluster-devel] [PATCH] gfs2: Null-check gl in gfs2_rgrp_dump()
2020-10-14 14:30 ` Andreas Gruenbacher
@ 2020-10-14 15:13 ` Andrew Price
0 siblings, 0 replies; 3+ messages in thread
From: Andrew Price @ 2020-10-14 15:13 UTC (permalink / raw)
To: cluster-devel.redhat.com
On 14/10/2020 15:30, Andreas Gruenbacher wrote:
> Hi Andy,
>
>> When an rindex entry is found to be corrupt, compute_bitstructs() calls
>> gfs2_consist_rgrpd() which calls gfs2_rgrp_dump() like this:
>>
>> gfs2_rgrp_dump(NULL, rgd->rd_gl, fs_id_buf);
>>
>> gfs2_rgrp_dump then dereferences the gl without checking it and we get
>>
>> BUG: KASAN: null-ptr-deref in gfs2_rgrp_dump+0x28/0x280
>>
>> because there's no rgrp glock involved while reading the rindex on mount.
>>
>> Add a NULL check for gl in gfs2_rgrp_dump() to allow the withdraw to
>> continue cleanly.
>
> thanks for looking into this. Shouldn't gfs2_rgrp_dump just take an
> rgrp argument as below though?
>
Yes, that's a much better arrangement. Built and tested against the
reproducer. Looks good.
Thanks,
Andy
> ---
> fs/gfs2/glops.c | 11 ++++++++++-
> fs/gfs2/rgrp.c | 9 +++------
> fs/gfs2/rgrp.h | 2 +-
> fs/gfs2/util.c | 2 +-
> 4 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index de1d5f1d9ff8..c2c90747d79b 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -227,6 +227,15 @@ static void rgrp_go_inval(struct gfs2_glock *gl, int flags)
> rgd->rd_flags &= ~GFS2_RDF_UPTODATE;
> }
>
> +static void gfs2_rgrp_go_dump(struct seq_file *seq, struct gfs2_glock *gl,
> + const char *fs_id_buf)
> +{
> + struct gfs2_rgrpd *rgd = gfs2_glock2rgrp(gl);
> +
> + if (rgd)
> + gfs2_rgrp_dump(seq, rgd, fs_id_buf);
> +}
> +
> static struct gfs2_inode *gfs2_glock2inode(struct gfs2_glock *gl)
> {
> struct gfs2_inode *ip;
> @@ -712,7 +721,7 @@ const struct gfs2_glock_operations gfs2_rgrp_glops = {
> .go_sync = rgrp_go_sync,
> .go_inval = rgrp_go_inval,
> .go_lock = gfs2_rgrp_go_lock,
> - .go_dump = gfs2_rgrp_dump,
> + .go_dump = gfs2_rgrp_go_dump,
> .go_type = LM_TYPE_RGRP,
> .go_flags = GLOF_LVB,
> };
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index 074f228ea839..1bba5a9d45fa 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -2209,20 +2209,17 @@ static void rgblk_free(struct gfs2_sbd *sdp, struct gfs2_rgrpd *rgd,
> /**
> * gfs2_rgrp_dump - print out an rgrp
> * @seq: The iterator
> - * @gl: The glock in question
> + * @rgd: The rgrp in question
> * @fs_id_buf: pointer to file system id (if requested)
> *
> */
>
> -void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_glock *gl,
> +void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_rgrpd *rgd,
> const char *fs_id_buf)
> {
> - struct gfs2_rgrpd *rgd = gl->gl_object;
> struct gfs2_blkreserv *trs;
> const struct rb_node *n;
>
> - if (rgd == NULL)
> - return;
> gfs2_print_dbg(seq, "%s R: n:%llu f:%02x b:%u/%u i:%u r:%u e:%u\n",
> fs_id_buf,
> (unsigned long long)rgd->rd_addr, rgd->rd_flags,
> @@ -2253,7 +2250,7 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
> (unsigned long long)rgd->rd_addr);
> fs_warn(sdp, "umount on all nodes and run fsck.gfs2 to fix the error\n");
> sprintf(fs_id_buf, "fsid=%s: ", sdp->sd_fsname);
> - gfs2_rgrp_dump(NULL, rgd->rd_gl, fs_id_buf);
> + gfs2_rgrp_dump(NULL, rgd, fs_id_buf);
> rgd->rd_flags |= GFS2_RDF_ERROR;
> }
>
> diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
> index a1d7e14fc55b..9a587ada51ed 100644
> --- a/fs/gfs2/rgrp.h
> +++ b/fs/gfs2/rgrp.h
> @@ -67,7 +67,7 @@ extern void gfs2_rlist_add(struct gfs2_inode *ip, struct gfs2_rgrp_list *rlist,
> extern void gfs2_rlist_alloc(struct gfs2_rgrp_list *rlist);
> extern void gfs2_rlist_free(struct gfs2_rgrp_list *rlist);
> extern u64 gfs2_ri_total(struct gfs2_sbd *sdp);
> -extern void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_glock *gl,
> +extern void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_rgrpd *rgd,
> const char *fs_id_buf);
> extern int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
> struct buffer_head *bh,
> diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
> index 1cd0328cae20..0fba3bf64189 100644
> --- a/fs/gfs2/util.c
> +++ b/fs/gfs2/util.c
> @@ -419,7 +419,7 @@ void gfs2_consist_rgrpd_i(struct gfs2_rgrpd *rgd,
> char fs_id_buf[sizeof(sdp->sd_fsname) + 7];
>
> sprintf(fs_id_buf, "fsid=%s: ", sdp->sd_fsname);
> - gfs2_rgrp_dump(NULL, rgd->rd_gl, fs_id_buf);
> + gfs2_rgrp_dump(NULL, rgd, fs_id_buf);
> gfs2_lm(sdp,
> "fatal: filesystem consistency error\n"
> " RG = %llu\n"
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-14 15:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-07 11:30 [Cluster-devel] [PATCH] gfs2: Null-check gl in gfs2_rgrp_dump() Andrew Price
2020-10-14 14:30 ` Andreas Gruenbacher
2020-10-14 15:13 ` Andrew Price
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).