[Cluster-devel] [bug report] GFS2: Withdraw for IO errors writing to the journal or statfs

[Cluster-devel] [bug report] GFS2: Withdraw for IO errors writing to the journal or statfs

[Dan Carpenter]

Hello Bob Peterson,

The patch 9bfef7554e20: "GFS2: Withdraw for IO errors writing to the
journal or statfs" from Aug 16, 2017, leads to the following static
checker warning:

	fs/gfs2/super.c:949 gfs2_sync_fs()
	error: we previously assumed 'sdp' could be null (see line 947)

fs/gfs2/super.c
   942  static int gfs2_sync_fs(struct super_block *sb, int wait)
   943  {
   944          struct gfs2_sbd *sdp = sb->s_fs_info;
   945  
   946          gfs2_quota_sync(sb, -1);
   947          if (wait && sdp)
                            ^^^
Existing code checks for NULL.

   948                  gfs2_log_flush(sdp, NULL, NORMAL_FLUSH);
   949          return sdp->sd_log_error;
                       ^^^^^^^^^^^^^^^^^
Patch adds unchecked dereference.

   950  }

regards,
dan carpenter

[Cluster-devel] [bug report] GFS2: Withdraw for IO errors writing to the journal or statfs

[Andrew Price]

On Thu, Aug 24, 2017 at 02:03:57PM +0300, Dan Carpenter wrote:
> Hello Bob Peterson,
> 
> The patch 9bfef7554e20: "GFS2: Withdraw for IO errors writing to the
> journal or statfs" from Aug 16, 2017, leads to the following static
> checker warning:
> 
> 	fs/gfs2/super.c:949 gfs2_sync_fs()
> 	error: we previously assumed 'sdp' could be null (see line 947)
> 
> fs/gfs2/super.c
>    942  static int gfs2_sync_fs(struct super_block *sb, int wait)
>    943  {
>    944          struct gfs2_sbd *sdp = sb->s_fs_info;
>    945  
>    946          gfs2_quota_sync(sb, -1);
>    947          if (wait && sdp)
>                             ^^^
> Existing code checks for NULL.

The NULL check seemed odd to me, and other ->sync_fs implementations don't check it, but digging through the history there's a reason it was added (see 9171f5a ). That said, I can't see the quota_off code path that it was guarding against any more, so perhaps it is no longer required.

Andy

> 
>    948                  gfs2_log_flush(sdp, NULL, NORMAL_FLUSH);
>    949          return sdp->sd_log_error;
>                        ^^^^^^^^^^^^^^^^^
> Patch adds unchecked dereference.

[Cluster-devel] [bug report] GFS2: Withdraw for IO errors writing to the journal or statfs

[Bob Peterson]

----- Original Message -----
| On Thu, Aug 24, 2017 at 02:03:57PM +0300, Dan Carpenter wrote:
| > Hello Bob Peterson,
| > 
| > The patch 9bfef7554e20: "GFS2: Withdraw for IO errors writing to the
| > journal or statfs" from Aug 16, 2017, leads to the following static
| > checker warning:
| > 
| > 	fs/gfs2/super.c:949 gfs2_sync_fs()
| > 	error: we previously assumed 'sdp' could be null (see line 947)
| > 
| > fs/gfs2/super.c
| >    942  static int gfs2_sync_fs(struct super_block *sb, int wait)
| >    943  {
| >    944          struct gfs2_sbd *sdp = sb->s_fs_info;
| >    945
| >    946          gfs2_quota_sync(sb, -1);
| >    947          if (wait && sdp)
| >                             ^^^
| > Existing code checks for NULL.
| 
| The NULL check seemed odd to me, and other ->sync_fs implementations don't
| check it, but digging through the history there's a reason it was added (see
| 9171f5a ). That said, I can't see the quota_off code path that it was
| guarding against any more, so perhaps it is no longer required.
| 
| Andy
| 
| > 
| >    948                  gfs2_log_flush(sdp, NULL, NORMAL_FLUSH);
| >    949          return sdp->sd_log_error;
| >                        ^^^^^^^^^^^^^^^^^
| > Patch adds unchecked dereference.
| 
Hi Andy (and Dan),

You're right. I did some research and this check for "&& sdp" seems completely
unnecessary now. In fact, I took it out and tried to recreate the problem
for which it was added (mounting with an invalid lock protocol) and it did
not recreate.

Therefore, I removed the offending check and did a force-push back to for-next.
Thanks; well spotted.

Regards,

Bob Peterson
Red Hat File Systems