From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bob Peterson Date: Wed, 14 Dec 2016 09:05:07 -0500 (EST) Subject: [Cluster-devel] [bug report] GFS2: Use resizable hash table for glocks In-Reply-To: <13637261.11653508.1481722917038.JavaMail.zimbra@redhat.com> References: <20161214085844.GA10659@elgon.mountain> <13637261.11653508.1481722917038.JavaMail.zimbra@redhat.com> Message-ID: <533897646.11668780.1481724307206.JavaMail.zimbra@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit ----- Original Message ----- | Hi Dan, | | ----- Original Message ----- | | Hello Bob Peterson, | | | | The patch 88ffbf3e037e: "GFS2: Use resizable hash table for glocks" | | from Mar 16, 2015, leads to the following static checker warning: | | | | fs/gfs2/glock.c:1813 gfs2_glock_iter_next() | | error: 'gi->gl' dereferencing possible ERR_PTR() | | | | fs/gfs2/glock.c | | 1803 static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi) | | 1804 { | | 1805 do { | | 1806 gi->gl = rhashtable_walk_next(&gi->hti); | | 1807 if (IS_ERR(gi->gl)) { | | 1808 if (PTR_ERR(gi->gl) == -EAGAIN) | | 1809 continue; | | | | This continue was probably intended to go to the top of the loop, but | | it's a do while loop so it actually drops down | | | | 1810 gi->gl = NULL; | | 1811 } | | 1812 /* Skip entries for other sb and dead entries */ | | 1813 } while ((gi->gl) && ((gi->sdp != gi->gl->gl_name.ln_sbd) | | || | | ^^^^^^^^ | | to here where we dereference gi->gl. It's weird that Smatch is only | | complaining about this now though... | | | | 1814 | | __lockref_is_dead(&gi->gl->gl_lockref))); | | 1815 } | | | | regards, | | dan carpenter | | Yes, that looks like a bug. Do you have a patch or should I patch it? | | It is weird that it's never been flagged before. Thank goodness the | circumstances that fail are unlikely: the table would have to be in | the middle of a resize to return -EAGAIN, and I think that's only | called when someone is dumping the glocks. Still, it's a bug, so we | need to fix it. | | Regards, | | Bob Peterson | Red Hat File Systems | Hi Dan, Does this look right? Bob Peterson Red Hat File Systems --- GFS2: Fix reference to ERR_PTR in gfs2_glock_iter_next This patch fixes a place where function gfs2_glock_iter_next can reference an invalid error pointer. Signed-off-by: Bob Peterson --- diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c index 14cbf60..68c089a 100644 --- a/fs/gfs2/glock.c +++ b/fs/gfs2/glock.c @@ -1808,10 +1808,13 @@ static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi) if (PTR_ERR(gi->gl) == -EAGAIN) continue; gi->gl = NULL; + return; } + if ((gi->sdp == gi->gl->gl_name.ln_sbd) && + !__lockref_is_dead(&gi->gl->gl_lockref)) + return; /* Skip entries for other sb and dead entries */ - } while ((gi->gl) && ((gi->sdp != gi->gl->gl_name.ln_sbd) || - __lockref_is_dead(&gi->gl->gl_lockref))); + } while ((gi->gl); } static void *gfs2_glock_seq_start(struct seq_file *seq, loff_t *pos)