[Cluster-devel] GFS2: Extract quota data from reservations structure (revert 5407e24)

[Cluster-devel] GFS2: Extract quota data from reservations structure (revert 5407e24)

[Dan Carpenter]

Hello Bob Peterson,

This is a semi-automatic email about new static checker warnings.

The patch ae3547b14335: "GFS2: Extract quota data from reservations 
structure (revert 5407e24)" from Oct 26, 2015, leads to the following 
Smatch complaint:

fs/gfs2/quota.c:573 gfs2_quota_hold()
	 error: we previously assumed 'ip->i_qadata' could be null (see line 567)

fs/gfs2/quota.c
   535  int gfs2_qa_alloc(struct gfs2_inode *ip)
   536  {
   537          int error = 0;
   538          struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
   539  
   540          if (sdp->sd_args.ar_quota != GFS2_QUOTA_ON)
   541                  return 0;

Success return here doesn't mean that ->i_qadata was allocated.

I feel like this is the wrong place for this check.  The function name
is "alloc" so it should allocate instead of making policy decisions.  We
do check for GFS2_QUOTA_ON in gfs2_quota_lock() but that check is too
late so we would have oopsed already.  Let's move that check in front of
the the gfs2_quota_hold() call and delete the check from here.

   542  
   543          down_write(&ip->i_rw_mutex);
   544          ip->i_qadata = kmem_cache_zalloc(gfs2_qadata_cachep, GFP_NOFS);
   545          if (!ip->i_qadata)
   546                  error = -ENOMEM;
   547          up_write(&ip->i_rw_mutex);
   548          return error;
   549  }
   550  
   551  void gfs2_qa_delete(struct gfs2_inode *ip, atomic_t *wcount)
   552  {
   553          down_write(&ip->i_rw_mutex);
   554          if (ip->i_qadata && ((wcount == NULL) || (atomic_read(wcount) <= 1))) {
   555                  kmem_cache_free(gfs2_qadata_cachep, ip->i_qadata);
   556                  ip->i_qadata = NULL;
   557          }
   558          up_write(&ip->i_rw_mutex);
   559  }
   560  
   561  int gfs2_quota_hold(struct gfs2_inode *ip, kuid_t uid, kgid_t gid)
   562  {
   563          struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
   564          struct gfs2_quota_data **qd;
   565          int error;
   566	
   567		if (ip->i_qadata == NULL) {
                    ^^^^^^^^^^^^^^^^^^^^
Check for NULL.

   568			error = gfs2_qa_alloc(ip);
   569			if (error)
   570				return error;
   571		}
   572	
   573		qd = ip->i_qadata->qa_qd;
                ^^^^^^^^^^^^^^^^^^^^^^^^
Dereference.

   574	
   575		if (gfs2_assert_warn(sdp, !ip->i_qadata->qa_qd_num) ||

regards,
dan carpenter

[Cluster-devel] GFS2: Extract quota data from reservations structure (revert 5407e24)

[Bob Peterson]

----- Original Message -----
> Hello Bob Peterson,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch ae3547b14335: "GFS2: Extract quota data from reservations
> structure (revert 5407e24)" from Oct 26, 2015, leads to the following
> Smatch complaint:
> 
> fs/gfs2/quota.c:573 gfs2_quota_hold()
> 	 error: we previously assumed 'ip->i_qadata' could be null (see line 567)
> 
> fs/gfs2/quota.c
>    535  int gfs2_qa_alloc(struct gfs2_inode *ip)
>    536  {
>    537          int error = 0;
>    538          struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
>    539
>    540          if (sdp->sd_args.ar_quota != GFS2_QUOTA_ON)
>    541                  return 0;
> 
> Success return here doesn't mean that ->i_qadata was allocated.
> 
> I feel like this is the wrong place for this check.  The function name
> is "alloc" so it should allocate instead of making policy decisions.  We
> do check for GFS2_QUOTA_ON in gfs2_quota_lock() but that check is too
> late so we would have oopsed already.  Let's move that check in front of
> the the gfs2_quota_hold() call and delete the check from here.
> 
>    542
>    543          down_write(&ip->i_rw_mutex);
>    544          ip->i_qadata = kmem_cache_zalloc(gfs2_qadata_cachep,
>    GFP_NOFS);
>    545          if (!ip->i_qadata)
>    546                  error = -ENOMEM;
>    547          up_write(&ip->i_rw_mutex);
>    548          return error;
>    549  }
>    550
>    551  void gfs2_qa_delete(struct gfs2_inode *ip, atomic_t *wcount)
>    552  {
>    553          down_write(&ip->i_rw_mutex);
>    554          if (ip->i_qadata && ((wcount == NULL) || (atomic_read(wcount)
>    <= 1))) {
>    555                  kmem_cache_free(gfs2_qadata_cachep, ip->i_qadata);
>    556                  ip->i_qadata = NULL;
>    557          }
>    558          up_write(&ip->i_rw_mutex);
>    559  }
>    560
>    561  int gfs2_quota_hold(struct gfs2_inode *ip, kuid_t uid, kgid_t gid)
>    562  {
>    563          struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
>    564          struct gfs2_quota_data **qd;
>    565          int error;
>    566
>    567		if (ip->i_qadata == NULL) {
>                     ^^^^^^^^^^^^^^^^^^^^
> Check for NULL.
> 
>    568			error = gfs2_qa_alloc(ip);
>    569			if (error)
>    570				return error;
>    571		}
>    572
>    573		qd = ip->i_qadata->qa_qd;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^
> Dereference.
> 
>    574
>    575		if (gfs2_assert_warn(sdp, !ip->i_qadata->qa_qd_num) ||
> 
> regards,
> dan carpenter
> 
Hi Dan,

Thanks for your comments.

I also found some serious bugs in the two GFS2 quota-related patches that
forced me to respin them. The revised versions of the patches should not
be prone to the problems you informed me of. If the static checker finds
a problem, it should be a false positive.

Let me know if the revised versions are acceptable. If not, let me know
what's wrong and I'll fix it.

Regards,

Bob Peterson
Red Hat File Systems