* [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).