Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
* re: NFS: handle source server reboot
@ 2019-11-15 10:05 Colin Ian King
  2019-11-15 19:51 ` Olga Kornievskaia
  0 siblings, 1 reply; 2+ messages in thread
From: Colin Ian King @ 2019-11-15 10:05 UTC (permalink / raw)
  To: Olga Kornievskaia, Trond Myklebust, Anna Schumaker, linux-nfs
  Cc: linux-kernel

Hi,

Static analysis with Coverity has detected a memory leak in the
following commit:

commit 0e65a32c8a569db363048e17a708b1a0913adbef
Author: Olga Kornievskaia <kolga@netapp.com>
Date:   Fri Jun 14 14:38:40 2019 -0400

    NFS: handle source server reboot

In function __nfs4_copy_file_range() in fs/nfs/nfs4file.c, analysis is
as follows:


155retry:
   5. Condition !nfs42_files_from_same_server(file_in, file_out), taking
false branch.
   9. Condition !nfs42_files_from_same_server(file_in, file_out), taking
false branch.

156        if (!nfs42_files_from_same_server(file_in, file_out)) {
157                /* for inter copy, if copy size if smaller than 12 RPC
158                 * payloads, fallback to traditional copy. There are
159                 * 14 RPCs during an NFSv4.x mount between source/dest
160                 * servers.
161                 */
162                if (sync ||
163                        count <= 14 *
NFS_SERVER(file_inode(file_in))->rsize)
164                        return -EOPNOTSUPP;
165                cn_resp = kzalloc(sizeof(struct nfs42_copy_notify_res),
166                                GFP_NOFS);
167                if (unlikely(cn_resp == NULL))
168                        return -ENOMEM;
169
170                ret = nfs42_proc_copy_notify(file_in, file_out, cn_resp);
171                if (ret) {
172                        ret = -EOPNOTSUPP;
173                        goto out;
174                }
175                nss = &cn_resp->cnr_src;
176                cnrs = &cn_resp->cnr_stateid;
177        }
178        ret = nfs42_proc_copy(file_in, pos_in, file_out, pos_out, count,
179                                nss, cnrs, sync);
180out:
   6. freed_arg: kfree frees cn_resp.

   CID 91571 (#1 of 1): Double free (USE_AFTER_FREE)10. double_free:
Calling kfree frees pointer cn_resp which has already been freed.

181        kfree(cn_resp);

   7. Condition ret == -11, taking true branch.

182        if (ret == -EAGAIN)
   8. Jumping to label retry.

183                goto retry;
184        return ret;
185}
186

On the 2nd iteration of the retry loop, cn_resp is being free'd twice if
the call to nfs42_files_from_same_server() returns zero since cn_resp is
not kalloc'd in the 2nd iteration. A naive fix would be to set cn_resp
to NULL after the kfree on line 181, but I'm not sure if there is a
better way to resolve this.

Colin

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: NFS: handle source server reboot
  2019-11-15 10:05 NFS: handle source server reboot Colin Ian King
@ 2019-11-15 19:51 ` Olga Kornievskaia
  0 siblings, 0 replies; 2+ messages in thread
From: Olga Kornievskaia @ 2019-11-15 19:51 UTC (permalink / raw)
  To: Colin Ian King
  Cc: Olga Kornievskaia, Trond Myklebust, Anna Schumaker, linux-nfs,
	linux-kernel

On Fri, Nov 15, 2019 at 5:06 AM Colin Ian King <colin.king@canonical.com> wrote:
>
> Hi,
>
> Static analysis with Coverity has detected a memory leak in the
> following commit:
>
> commit 0e65a32c8a569db363048e17a708b1a0913adbef
> Author: Olga Kornievskaia <kolga@netapp.com>
> Date:   Fri Jun 14 14:38:40 2019 -0400
>
>     NFS: handle source server reboot
>
> In function __nfs4_copy_file_range() in fs/nfs/nfs4file.c, analysis is
> as follows:
>
>
> 155retry:
>    5. Condition !nfs42_files_from_same_server(file_in, file_out), taking
> false branch.
>    9. Condition !nfs42_files_from_same_server(file_in, file_out), taking
> false branch.
>
> 156        if (!nfs42_files_from_same_server(file_in, file_out)) {
> 157                /* for inter copy, if copy size if smaller than 12 RPC
> 158                 * payloads, fallback to traditional copy. There are
> 159                 * 14 RPCs during an NFSv4.x mount between source/dest
> 160                 * servers.
> 161                 */
> 162                if (sync ||
> 163                        count <= 14 *
> NFS_SERVER(file_inode(file_in))->rsize)
> 164                        return -EOPNOTSUPP;
> 165                cn_resp = kzalloc(sizeof(struct nfs42_copy_notify_res),
> 166                                GFP_NOFS);
> 167                if (unlikely(cn_resp == NULL))
> 168                        return -ENOMEM;
> 169
> 170                ret = nfs42_proc_copy_notify(file_in, file_out, cn_resp);
> 171                if (ret) {
> 172                        ret = -EOPNOTSUPP;
> 173                        goto out;
> 174                }
> 175                nss = &cn_resp->cnr_src;
> 176                cnrs = &cn_resp->cnr_stateid;
> 177        }
> 178        ret = nfs42_proc_copy(file_in, pos_in, file_out, pos_out, count,
> 179                                nss, cnrs, sync);
> 180out:
>    6. freed_arg: kfree frees cn_resp.
>
>    CID 91571 (#1 of 1): Double free (USE_AFTER_FREE)10. double_free:
> Calling kfree frees pointer cn_resp which has already been freed.
>
> 181        kfree(cn_resp);
>
>    7. Condition ret == -11, taking true branch.
>
> 182        if (ret == -EAGAIN)
>    8. Jumping to label retry.
>
> 183                goto retry;
> 184        return ret;
> 185}
> 186
>
> On the 2nd iteration of the retry loop, cn_resp is being free'd twice if
> the call to nfs42_files_from_same_server() returns zero since cn_resp is
> not kalloc'd in the 2nd iteration. A naive fix would be to set cn_resp
> to NULL after the kfree on line 181, but I'm not sure if there is a
> better way to resolve this.
>

If a definition of double free include freeing a null pointer twice,
then I agree this is a valid catch. Cases are when servers are the
same kfree(cn_resp) is called with a null argument and if retried will
be called with a null argument again (since kfree doesn't care about
null it shouldn't be a problem). If servers are not the same, then
memory is allocated then freed then allocated again. When code is
re-executed (on retry), it's not possible for condition to change from
servers being same or different.

I'll send a patch that conditions the kfree() to only when it was
allocated. Hopefully, it won't trip the analysis that way.

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-15 10:05 NFS: handle source server reboot Colin Ian King
2019-11-15 19:51 ` Olga Kornievskaia

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org
	public-inbox-index linux-nfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git