[bug report] NFSD introduce async copy feature

[bug report] NFSD introduce async copy feature

[Dan Carpenter]

Hello Olga Kornievskaia,

The patch e0639dc5805a: "NFSD introduce async copy feature" from Jul
20, 2018, leads to the following static checker warning:

	fs/nfsd/nfs4proc.c:1494 nfsd4_do_async_copy()
	error: we previously assumed 'copy->nf_src' could be null (see line 1464)

fs/nfsd/nfs4proc.c
  1460          struct nfsd4_copy *cb_copy;
  1461  
  1462          if (!copy->cp_intra) { /* Inter server SSC */
  1463                  copy->nf_src = kzalloc(sizeof(struct nfsd_file), GFP_KERNEL);
  1464                  if (!copy->nf_src) {
                             ^^^^^^^^^^^^
Check for NULL (allocation failure).

  1465                          copy->nfserr = nfserr_serverfault;
  1466                          nfsd4_interssc_disconnect(copy->ss_mnt);
  1467                          goto do_callback;
  1468                  }
  1469                  copy->nf_src->nf_file = nfs42_ssc_open(copy->ss_mnt, &copy->c_fh,
  1470                                                &copy->stateid);
  1471                  if (IS_ERR(copy->nf_src->nf_file)) {
  1472                          kfree(copy->nf_src);
  1473                          copy->nfserr = nfserr_offload_denied;
  1474                          nfsd4_interssc_disconnect(copy->ss_mnt);
  1475                          goto do_callback;
  1476                  }
  1477          }
  1478  
  1479          copy->nfserr = nfsd4_do_copy(copy, 0);
  1480  do_callback:
  1481          cb_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
  1482          if (!cb_copy)
  1483                  goto out;
  1484          memcpy(&cb_copy->cp_res, &copy->cp_res, sizeof(copy->cp_res));
  1485          cb_copy->cp_clp = copy->cp_clp;
  1486          cb_copy->nfserr = copy->nfserr;
  1487          memcpy(&cb_copy->fh, &copy->fh, sizeof(copy->fh));
  1488          nfsd4_init_cb(&cb_copy->cp_cb, cb_copy->cp_clp,
  1489                          &nfsd4_cb_offload_ops, NFSPROC4_CLNT_CB_OFFLOAD);
  1490          nfsd4_run_cb(&cb_copy->cp_cb);
  1491  out:
  1492          if (!copy->cp_intra)
  1493                  kfree(copy->nf_src);
                              ^^^^^^^^^^^^
  1494          cleanup_async_copy(copy);
                                   ^^^^
copy->nf_src can be NULL or it can be freed so this cleanup function
is going to crash.

  1495          return 0;
  1496  }

regards,
dan carpenter

[bug report] NFSD introduce async copy feature

[Dan Carpenter]

Hello Olga Kornievskaia,

The patch e0639dc5805a: "NFSD introduce async copy feature" from Jul
20, 2018, leads to the following static checker warning:

	fs/nfsd/nfs4proc.c:1544 nfsd4_copy()
	error: '__memcpy()' '&copy->cp_res.cb_stateid' too small (16 vs 24)

fs/nfsd/nfs4proc.c
  1508  static __be32
  1509  nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
  1510                  union nfsd4_op_u *u)
  1511  {
  1512          struct nfsd4_copy *copy = &u->copy;
  1513          __be32 status;
  1514          struct nfsd4_copy *async_copy = NULL;
  1515  
  1516          if (!copy->cp_intra) { /* Inter server SSC */
  1517                  if (!inter_copy_offload_enable || copy->cp_synchronous) {
  1518                          status = nfserr_notsupp;
  1519                          goto out;
  1520                  }
  1521                  status = nfsd4_setup_inter_ssc(rqstp, cstate, copy,
  1522                                  &copy->ss_mnt);
  1523                  if (status)
  1524                          return nfserr_offload_denied;
  1525          } else {
  1526                  status = nfsd4_setup_intra_ssc(rqstp, cstate, copy);
  1527                  if (status)
  1528                          return status;
  1529          }
  1530  
  1531          copy->cp_clp = cstate->clp;
  1532          memcpy(&copy->fh, &cstate->current_fh.fh_handle,
  1533                  sizeof(struct knfsd_fh));
  1534          if (!copy->cp_synchronous) {
  1535                  struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
  1536  
  1537                  status = nfserrno(-ENOMEM);
  1538                  async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
  1539                  if (!async_copy)
  1540                          goto out_err;
  1541                  if (!nfs4_init_copy_state(nn, copy))
  1542                          goto out_err;
  1543                  refcount_set(&async_copy->refcount, 1);
  1544                  memcpy(&copy->cp_res.cb_stateid, &copy->cp_stateid,
  1545                          sizeof(copy->cp_stateid));

It took me a while to spot the cb_ vs cp_...  :P

The copy->cp_stateid looks like this: fs/nfsd/state.h
    59  typedef struct {
    60          stateid_t               stid;
    61  #define NFS4_COPY_STID 1
    62  #define NFS4_COPYNOTIFY_STID 2
    63          unsigned char           sc_type;
    64          refcount_t              sc_count;
    65  } copy_stateid_t;

The .cb_stateid is just the stateid without the sc_type or the
refcounting.  I suspect we should only be copying the stateid.

  1546                  dup_copy_fields(copy, async_copy);
  1547                  async_copy->copy_task = kthread_create(nfsd4_do_async_copy,
  1548                                  async_copy, "%s", "copy thread");
  1549                  if (IS_ERR(async_copy->copy_task))
  1550                          goto out_err;
  1551                  spin_lock(&async_copy->cp_clp->async_lock);
  1552                  list_add(&async_copy->copies,

regards,
dan carpenter

[bug report] NFSD introduce async copy feature

[Dan Carpenter]

Hello Olga Kornievskaia,

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

The patch e0639dc5805a: "NFSD introduce async copy feature" from Jul 
20, 2018, leads to the following Smatch complaint:

    fs/nfsd/nfs4proc.c:1555 nfsd4_copy()
     error: we previously assumed 'async_copy' could be null (see line 1529)

fs/nfsd/nfs4proc.c
  1528			async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
  1529			if (!async_copy)
  1530				goto out_err;
                                ^^^^^^^^^^^^
There are a couple error paths where async_copy is NULL.

  1531			if (!nfs4_init_copy_state(nn, copy))
  1532				goto out_err;
  1533			refcount_set(&async_copy->refcount, 1);
  1534			memcpy(&copy->cp_res.cb_stateid, &copy->cp_stateid,
  1535				sizeof(copy->cp_stateid));
  1536			status = dup_copy_fields(copy, async_copy);
  1537			if (status)
  1538				goto out_err;
  1539			async_copy->copy_task = kthread_create(nfsd4_do_async_copy,
  1540					async_copy, "%s", "copy thread");
  1541			if (IS_ERR(async_copy->copy_task))
  1542				goto out_err;
  1543			spin_lock(&async_copy->cp_clp->async_lock);
  1544			list_add(&async_copy->copies,
  1545					&async_copy->cp_clp->async_copies);
  1546			spin_unlock(&async_copy->cp_clp->async_lock);
  1547			wake_up_process(async_copy->copy_task);
  1548			status = nfs_ok;
  1549		} else {
  1550			status = nfsd4_do_copy(copy, 1);
  1551		}
  1552	out:
  1553		return status;
  1554	out_err:
  1555		cleanup_async_copy(async_copy);
                                   ^^^^^^^^^^
Dereferenced inside the function.

  1556		status = nfserrno(-ENOMEM);
  1557		if (!copy->cp_intra)

regards,
dan carpenter