From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bob Peterson <rpeterso@redhat.com>
Date: Thu, 19 Nov 2015 08:55:25 -0500 (EST)
Subject: [Cluster-devel] GFS2: Extract quota data from reservations
 structure (revert 5407e24)
In-Reply-To: <20151118224433.GA25959@mwanda>
References: <20151118224433.GA25959@mwanda>
Message-ID: <190908777.13661056.1447941325105.JavaMail.zimbra@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

----- 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