| Message ID | 20181019152905.32418-8-olga.kornievskaia@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | server-side support for "inter" SSC copy | expand |
On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote: > Note: followed conventions and have struct nfsd4_compoundargs pointer as a > parameter even though it is unused. It's used--see the definition of READ_BUF. (I'm not a fan of those macros and they'll probably be replaced some day.) --b. > > Signed-off-by: Andy Adamson <andros@netapp.com> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > --- > fs/nfsd/nfs4xdr.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- > fs/nfsd/xdr4.h | 1 + > 2 files changed, 70 insertions(+), 2 deletions(-) > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index 3de42a7..9f6886f 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -40,6 +40,7 @@ > #include <linux/utsname.h> > #include <linux/pagemap.h> > #include <linux/sunrpc/svcauth_gss.h> > +#include <linux/sunrpc/addr.h> > > #include "idmap.h" > #include "acl.h" > @@ -1743,11 +1744,58 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str > DECODE_TAIL; > } > > +static __be32 nfsd4_decode_nl4_server(struct nfsd4_compoundargs *argp, > + struct nl4_server *ns) > +{ > + DECODE_HEAD; > + struct nfs42_netaddr *naddr; > + > + READ_BUF(4); > + ns->nl4_type = be32_to_cpup(p++); > + > + /* currently support for 1 inter-server source server */ > + switch (ns->nl4_type) { > + case NL4_NAME: > + case NL4_URL: > + READ_BUF(4); > + ns->u.nl4_str_sz = be32_to_cpup(p++); > + if (ns->u.nl4_str_sz > NFS4_OPAQUE_LIMIT) > + goto xdr_error; > + > + READ_BUF(ns->u.nl4_str_sz); > + COPYMEM(ns->u.nl4_str, > + ns->u.nl4_str_sz); > + break; > + case NL4_NETADDR: > + naddr = &ns->u.nl4_addr; > + > + READ_BUF(4); > + naddr->netid_len = be32_to_cpup(p++); > + if (naddr->netid_len > RPCBIND_MAXNETIDLEN) > + goto xdr_error; > + > + READ_BUF(naddr->netid_len + 4); /* 4 for uaddr len */ > + COPYMEM(naddr->netid, naddr->netid_len); > + > + naddr->addr_len = be32_to_cpup(p++); > + if (naddr->addr_len > RPCBIND_MAXUADDRLEN) > + goto xdr_error; > + > + READ_BUF(naddr->addr_len); > + COPYMEM(naddr->addr, naddr->addr_len); > + break; > + default: > + goto xdr_error; > + } > + DECODE_TAIL; > +} > + > static __be32 > nfsd4_decode_copy(struct nfsd4_compoundargs *argp, struct nfsd4_copy *copy) > { > DECODE_HEAD; > - unsigned int tmp; > + struct nl4_server *ns; > + int i, count; > > status = nfsd4_decode_stateid(argp, ©->cp_src_stateid); > if (status) > @@ -1762,8 +1810,24 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str > p = xdr_decode_hyper(p, ©->cp_count); > p++; /* ca_consecutive: we always do consecutive copies */ > copy->cp_synchronous = be32_to_cpup(p++); > - tmp = be32_to_cpup(p); /* Source server list not supported */ > + count = be32_to_cpup(p++); > + > + if (count == 0) /* intra-server copy */ > + goto intra; > > + /* decode all the supplied server addresses but use first */ > + copy->cp_src = kmalloc(count * sizeof(struct nl4_server), GFP_KERNEL); > + if (copy->cp_src == NULL) > + return nfserrno(-ENOMEM); > + > + ns = copy->cp_src; > + for (i = 0; i < count; i++) { > + status = nfsd4_decode_nl4_server(argp, ns); > + if (status) > + return status; > + ns++; > + } > +intra: > DECODE_TAIL; > } > > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp, > p = xdr_reserve_space(&resp->xdr, 4 + 4); > *p++ = xdr_one; /* cr_consecutive */ > *p++ = cpu_to_be32(copy->cp_synchronous); > + > + /* allocated in nfsd4_decode_copy */ > + kfree(copy->cp_src); > return 0; > } > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > index feeb6d4..b4d1140 100644 > --- a/fs/nfsd/xdr4.h > +++ b/fs/nfsd/xdr4.h > @@ -521,6 +521,7 @@ struct nfsd4_copy { > u64 cp_src_pos; > u64 cp_dst_pos; > u64 cp_count; > + struct nl4_server *cp_src; > > /* both */ > bool cp_synchronous; > -- > 1.8.3.1
On Thu, Nov 1, 2018 at 4:48 PM J. Bruce Fields <bfields@fieldses.org> wrote: > > On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote: > > Note: followed conventions and have struct nfsd4_compoundargs pointer as a > > parameter even though it is unused. > > It's used--see the definition of READ_BUF. Sigh. It's a leftover comment from Andy. I'll remove it. > (I'm not a fan of those macros and they'll probably be replaced some > day.) READ_BUF is used thru out so I'm assuming you are not against this patch using it, correct? > > --b. > > > > > Signed-off-by: Andy Adamson <andros@netapp.com> > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > --- > > fs/nfsd/nfs4xdr.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- > > fs/nfsd/xdr4.h | 1 + > > 2 files changed, 70 insertions(+), 2 deletions(-) > > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > index 3de42a7..9f6886f 100644 > > --- a/fs/nfsd/nfs4xdr.c > > +++ b/fs/nfsd/nfs4xdr.c > > @@ -40,6 +40,7 @@ > > #include <linux/utsname.h> > > #include <linux/pagemap.h> > > #include <linux/sunrpc/svcauth_gss.h> > > +#include <linux/sunrpc/addr.h> > > > > #include "idmap.h" > > #include "acl.h" > > @@ -1743,11 +1744,58 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str > > DECODE_TAIL; > > } > > > > +static __be32 nfsd4_decode_nl4_server(struct nfsd4_compoundargs *argp, > > + struct nl4_server *ns) > > +{ > > + DECODE_HEAD; > > + struct nfs42_netaddr *naddr; > > + > > + READ_BUF(4); > > + ns->nl4_type = be32_to_cpup(p++); > > + > > + /* currently support for 1 inter-server source server */ > > + switch (ns->nl4_type) { > > + case NL4_NAME: > > + case NL4_URL: > > + READ_BUF(4); > > + ns->u.nl4_str_sz = be32_to_cpup(p++); > > + if (ns->u.nl4_str_sz > NFS4_OPAQUE_LIMIT) > > + goto xdr_error; > > + > > + READ_BUF(ns->u.nl4_str_sz); > > + COPYMEM(ns->u.nl4_str, > > + ns->u.nl4_str_sz); > > + break; > > + case NL4_NETADDR: > > + naddr = &ns->u.nl4_addr; > > + > > + READ_BUF(4); > > + naddr->netid_len = be32_to_cpup(p++); > > + if (naddr->netid_len > RPCBIND_MAXNETIDLEN) > > + goto xdr_error; > > + > > + READ_BUF(naddr->netid_len + 4); /* 4 for uaddr len */ > > + COPYMEM(naddr->netid, naddr->netid_len); > > + > > + naddr->addr_len = be32_to_cpup(p++); > > + if (naddr->addr_len > RPCBIND_MAXUADDRLEN) > > + goto xdr_error; > > + > > + READ_BUF(naddr->addr_len); > > + COPYMEM(naddr->addr, naddr->addr_len); > > + break; > > + default: > > + goto xdr_error; > > + } > > + DECODE_TAIL; > > +} > > + > > static __be32 > > nfsd4_decode_copy(struct nfsd4_compoundargs *argp, struct nfsd4_copy *copy) > > { > > DECODE_HEAD; > > - unsigned int tmp; > > + struct nl4_server *ns; > > + int i, count; > > > > status = nfsd4_decode_stateid(argp, ©->cp_src_stateid); > > if (status) > > @@ -1762,8 +1810,24 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str > > p = xdr_decode_hyper(p, ©->cp_count); > > p++; /* ca_consecutive: we always do consecutive copies */ > > copy->cp_synchronous = be32_to_cpup(p++); > > - tmp = be32_to_cpup(p); /* Source server list not supported */ > > + count = be32_to_cpup(p++); > > + > > + if (count == 0) /* intra-server copy */ > > + goto intra; > > > > + /* decode all the supplied server addresses but use first */ > > + copy->cp_src = kmalloc(count * sizeof(struct nl4_server), GFP_KERNEL); > > + if (copy->cp_src == NULL) > > + return nfserrno(-ENOMEM); > > + > > + ns = copy->cp_src; > > + for (i = 0; i < count; i++) { > > + status = nfsd4_decode_nl4_server(argp, ns); > > + if (status) > > + return status; > > + ns++; > > + } > > +intra: > > DECODE_TAIL; > > } > > > > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp, > > p = xdr_reserve_space(&resp->xdr, 4 + 4); > > *p++ = xdr_one; /* cr_consecutive */ > > *p++ = cpu_to_be32(copy->cp_synchronous); > > + > > + /* allocated in nfsd4_decode_copy */ > > + kfree(copy->cp_src); > > return 0; > > } > > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > index feeb6d4..b4d1140 100644 > > --- a/fs/nfsd/xdr4.h > > +++ b/fs/nfsd/xdr4.h > > @@ -521,6 +521,7 @@ struct nfsd4_copy { > > u64 cp_src_pos; > > u64 cp_dst_pos; > > u64 cp_count; > > + struct nl4_server *cp_src; > > > > /* both */ > > bool cp_synchronous; > > -- > > 1.8.3.1
On Thu, Nov 01, 2018 at 05:00:46PM -0400, Olga Kornievskaia wrote: > On Thu, Nov 1, 2018 at 4:48 PM J. Bruce Fields <bfields@fieldses.org> wrote: > > > > On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote: > > > Note: followed conventions and have struct nfsd4_compoundargs pointer as a > > > parameter even though it is unused. > > > > It's used--see the definition of READ_BUF. > > Sigh. It's a leftover comment from Andy. I'll remove it. > > > (I'm not a fan of those macros and they'll probably be replaced some > > day.) > > READ_BUF is used thru out so I'm assuming you are not against this > patch using it, correct? Correct, replacing those macros is a project for another day. --b. > > > > > --b. > > > > > > > > Signed-off-by: Andy Adamson <andros@netapp.com> > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > > --- > > > fs/nfsd/nfs4xdr.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- > > > fs/nfsd/xdr4.h | 1 + > > > 2 files changed, 70 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > > index 3de42a7..9f6886f 100644 > > > --- a/fs/nfsd/nfs4xdr.c > > > +++ b/fs/nfsd/nfs4xdr.c > > > @@ -40,6 +40,7 @@ > > > #include <linux/utsname.h> > > > #include <linux/pagemap.h> > > > #include <linux/sunrpc/svcauth_gss.h> > > > +#include <linux/sunrpc/addr.h> > > > > > > #include "idmap.h" > > > #include "acl.h" > > > @@ -1743,11 +1744,58 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str > > > DECODE_TAIL; > > > } > > > > > > +static __be32 nfsd4_decode_nl4_server(struct nfsd4_compoundargs *argp, > > > + struct nl4_server *ns) > > > +{ > > > + DECODE_HEAD; > > > + struct nfs42_netaddr *naddr; > > > + > > > + READ_BUF(4); > > > + ns->nl4_type = be32_to_cpup(p++); > > > + > > > + /* currently support for 1 inter-server source server */ > > > + switch (ns->nl4_type) { > > > + case NL4_NAME: > > > + case NL4_URL: > > > + READ_BUF(4); > > > + ns->u.nl4_str_sz = be32_to_cpup(p++); > > > + if (ns->u.nl4_str_sz > NFS4_OPAQUE_LIMIT) > > > + goto xdr_error; > > > + > > > + READ_BUF(ns->u.nl4_str_sz); > > > + COPYMEM(ns->u.nl4_str, > > > + ns->u.nl4_str_sz); > > > + break; > > > + case NL4_NETADDR: > > > + naddr = &ns->u.nl4_addr; > > > + > > > + READ_BUF(4); > > > + naddr->netid_len = be32_to_cpup(p++); > > > + if (naddr->netid_len > RPCBIND_MAXNETIDLEN) > > > + goto xdr_error; > > > + > > > + READ_BUF(naddr->netid_len + 4); /* 4 for uaddr len */ > > > + COPYMEM(naddr->netid, naddr->netid_len); > > > + > > > + naddr->addr_len = be32_to_cpup(p++); > > > + if (naddr->addr_len > RPCBIND_MAXUADDRLEN) > > > + goto xdr_error; > > > + > > > + READ_BUF(naddr->addr_len); > > > + COPYMEM(naddr->addr, naddr->addr_len); > > > + break; > > > + default: > > > + goto xdr_error; > > > + } > > > + DECODE_TAIL; > > > +} > > > + > > > static __be32 > > > nfsd4_decode_copy(struct nfsd4_compoundargs *argp, struct nfsd4_copy *copy) > > > { > > > DECODE_HEAD; > > > - unsigned int tmp; > > > + struct nl4_server *ns; > > > + int i, count; > > > > > > status = nfsd4_decode_stateid(argp, ©->cp_src_stateid); > > > if (status) > > > @@ -1762,8 +1810,24 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str > > > p = xdr_decode_hyper(p, ©->cp_count); > > > p++; /* ca_consecutive: we always do consecutive copies */ > > > copy->cp_synchronous = be32_to_cpup(p++); > > > - tmp = be32_to_cpup(p); /* Source server list not supported */ > > > + count = be32_to_cpup(p++); > > > + > > > + if (count == 0) /* intra-server copy */ > > > + goto intra; > > > > > > + /* decode all the supplied server addresses but use first */ > > > + copy->cp_src = kmalloc(count * sizeof(struct nl4_server), GFP_KERNEL); > > > + if (copy->cp_src == NULL) > > > + return nfserrno(-ENOMEM); > > > + > > > + ns = copy->cp_src; > > > + for (i = 0; i < count; i++) { > > > + status = nfsd4_decode_nl4_server(argp, ns); > > > + if (status) > > > + return status; > > > + ns++; > > > + } > > > +intra: > > > DECODE_TAIL; > > > } > > > > > > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp, > > > p = xdr_reserve_space(&resp->xdr, 4 + 4); > > > *p++ = xdr_one; /* cr_consecutive */ > > > *p++ = cpu_to_be32(copy->cp_synchronous); > > > + > > > + /* allocated in nfsd4_decode_copy */ > > > + kfree(copy->cp_src); > > > return 0; > > > } > > > > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > > index feeb6d4..b4d1140 100644 > > > --- a/fs/nfsd/xdr4.h > > > +++ b/fs/nfsd/xdr4.h > > > @@ -521,6 +521,7 @@ struct nfsd4_copy { > > > u64 cp_src_pos; > > > u64 cp_dst_pos; > > > u64 cp_count; > > > + struct nl4_server *cp_src; > > > > > > /* both */ > > > bool cp_synchronous; > > > -- > > > 1.8.3.1
On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote: > @@ -1762,8 +1810,24 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str > p = xdr_decode_hyper(p, ©->cp_count); > p++; /* ca_consecutive: we always do consecutive copies */ > copy->cp_synchronous = be32_to_cpup(p++); > - tmp = be32_to_cpup(p); /* Source server list not supported */ > + count = be32_to_cpup(p++); > + > + if (count == 0) /* intra-server copy */ > + goto intra; > > + /* decode all the supplied server addresses but use first */ > + copy->cp_src = kmalloc(count * sizeof(struct nl4_server), GFP_KERNEL); The client could pass an arbitrarily large count here. I don't know if the ability to force the server to attempt a large kmalloc() is really useful to an attacker, but I'd definitely rather not allow it. Possibly more serious: if that multiplication overflows, then in theory it might be possible to make the kmalloc() succeed and allocate too little memory, after which the following loop could overwrite memory past the end of the allocation. As long as we're only using the first address, maybe it's simplest just not to bother allocating memory for the rest. Just copy the first one, and for the rest call nfsd4_decode_nl4_server() with a dummy struct nl4_server. --b. > + if (copy->cp_src == NULL) > + return nfserrno(-ENOMEM); > + > + ns = copy->cp_src; > + for (i = 0; i < count; i++) { > + status = nfsd4_decode_nl4_server(argp, ns); > + if (status) > + return status; > + ns++; > + } > +intra: > DECODE_TAIL; > } > > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp, > p = xdr_reserve_space(&resp->xdr, 4 + 4); > *p++ = xdr_one; /* cr_consecutive */ > *p++ = cpu_to_be32(copy->cp_synchronous); > + > + /* allocated in nfsd4_decode_copy */ > + kfree(copy->cp_src); > return 0; > } > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > index feeb6d4..b4d1140 100644 > --- a/fs/nfsd/xdr4.h > +++ b/fs/nfsd/xdr4.h > @@ -521,6 +521,7 @@ struct nfsd4_copy { > u64 cp_src_pos; > u64 cp_dst_pos; > u64 cp_count; > + struct nl4_server *cp_src; > > /* both */ > bool cp_synchronous; > -- > 1.8.3.1
On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote: > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp, > p = xdr_reserve_space(&resp->xdr, 4 + 4); > *p++ = xdr_one; /* cr_consecutive */ > *p++ = cpu_to_be32(copy->cp_synchronous); > + > + /* allocated in nfsd4_decode_copy */ > + kfree(copy->cp_src); This can result in a leak--for example, if we decode the compound succesfully, but processing fails before we could to this op, then we'll never call this encoder, so we'll allocate without freeing. I think simplest would be to replace this: > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > index feeb6d4..b4d1140 100644 > --- a/fs/nfsd/xdr4.h > +++ b/fs/nfsd/xdr4.h > @@ -521,6 +521,7 @@ struct nfsd4_copy { > u64 cp_src_pos; > u64 cp_dst_pos; > u64 cp_count; > + struct nl4_server *cp_src; by just a struct nl4_server cp_src; since it sounds like you really only need one of them, not a whole array (at least for now). --b. > > /* both */ > bool cp_synchronous; > -- > 1.8.3.1
On Fri, Nov 2, 2018 at 11:46 AM J. Bruce Fields <bfields@fieldses.org> wrote: > > On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote: > > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp, > > p = xdr_reserve_space(&resp->xdr, 4 + 4); > > *p++ = xdr_one; /* cr_consecutive */ > > *p++ = cpu_to_be32(copy->cp_synchronous); > > + > > + /* allocated in nfsd4_decode_copy */ > > + kfree(copy->cp_src); > > This can result in a leak--for example, if we decode the compound > succesfully, but processing fails before we could to this op, then we'll > never call this encoder, so we'll allocate without freeing. > > I think simplest would be to replace this: > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > index feeb6d4..b4d1140 100644 > > --- a/fs/nfsd/xdr4.h > > +++ b/fs/nfsd/xdr4.h > > @@ -521,6 +521,7 @@ struct nfsd4_copy { > > u64 cp_src_pos; > > u64 cp_dst_pos; > > u64 cp_count; > > + struct nl4_server *cp_src; > > by just a > > struct nl4_server cp_src; > > since it sounds like you really only need one of them, not a whole array > (at least for now). So this is problematic as the presence of this memory is what is used to distinguish "inter" from "intra". Can things really fail between the xdr and calling of the operation? What gets freed in the encoder is the "copy" of the what was decoded in the decoder. But really freeing in the encoder is the wrong place. Encoder doesn't need to free. I already free the "copy" of the copy->cp_src in the cleanup_async_copy(). However, what is missing is freeing the original copy->cp_src which needs to be freed in the dup_copy_fields(). To clarify: copy->cp_src gets allocated in the decoder during the process of the copy: 1. it gets copied to the kthread and the original copy->cp_src needs to be freed. Or during any error it will be freed. 2. cleanup_async_copy frees the copy of the copy->cp_src. (need to remove the kfree from the encoder). > > --b. > > > > > /* both */ > > bool cp_synchronous; > > -- > > 1.8.3.1
On Fri, Nov 2, 2018 at 10:04 AM J. Bruce Fields <bfields@fieldses.org> wrote: > > On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote: > > @@ -1762,8 +1810,24 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str > > p = xdr_decode_hyper(p, ©->cp_count); > > p++; /* ca_consecutive: we always do consecutive copies */ > > copy->cp_synchronous = be32_to_cpup(p++); > > - tmp = be32_to_cpup(p); /* Source server list not supported */ > > + count = be32_to_cpup(p++); > > + > > + if (count == 0) /* intra-server copy */ > > + goto intra; > > > > + /* decode all the supplied server addresses but use first */ > > + copy->cp_src = kmalloc(count * sizeof(struct nl4_server), GFP_KERNEL); > > The client could pass an arbitrarily large count here. I don't know if > the ability to force the server to attempt a large kmalloc() is really > useful to an attacker, but I'd definitely rather not allow it. > > Possibly more serious: if that multiplication overflows, then in theory > it might be possible to make the kmalloc() succeed and allocate too > little memory, after which the following loop could overwrite memory > past the end of the allocation. > > As long as we're only using the first address, maybe it's simplest just > not to bother allocating memory for the rest. Just copy the first one, > and for the rest call nfsd4_decode_nl4_server() with a dummy struct > nl4_server. Ok will try. > > --b. > > > + if (copy->cp_src == NULL) > > + return nfserrno(-ENOMEM); > > + > > + ns = copy->cp_src; > > + for (i = 0; i < count; i++) { > > + status = nfsd4_decode_nl4_server(argp, ns); > > + if (status) > > + return status; > > + ns++; > > + } > > +intra: > > DECODE_TAIL; > > } > > > > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp, > > p = xdr_reserve_space(&resp->xdr, 4 + 4); > > *p++ = xdr_one; /* cr_consecutive */ > > *p++ = cpu_to_be32(copy->cp_synchronous); > > + > > + /* allocated in nfsd4_decode_copy */ > > + kfree(copy->cp_src); > > return 0; > > } > > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > index feeb6d4..b4d1140 100644 > > --- a/fs/nfsd/xdr4.h > > +++ b/fs/nfsd/xdr4.h > > @@ -521,6 +521,7 @@ struct nfsd4_copy { > > u64 cp_src_pos; > > u64 cp_dst_pos; > > u64 cp_count; > > + struct nl4_server *cp_src; > > > > /* both */ > > bool cp_synchronous; > > -- > > 1.8.3.1
On Fri, Nov 02, 2018 at 12:35:26PM -0400, Olga Kornievskaia wrote: > On Fri, Nov 2, 2018 at 11:46 AM J. Bruce Fields <bfields@fieldses.org> wrote: > > > > On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote: > > > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp, > > > p = xdr_reserve_space(&resp->xdr, 4 + 4); > > > *p++ = xdr_one; /* cr_consecutive */ > > > *p++ = cpu_to_be32(copy->cp_synchronous); > > > + > > > + /* allocated in nfsd4_decode_copy */ > > > + kfree(copy->cp_src); > > > > This can result in a leak--for example, if we decode the compound > > succesfully, but processing fails before we could to this op, then we'll > > never call this encoder, so we'll allocate without freeing. > > > > I think simplest would be to replace this: > > > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > > index feeb6d4..b4d1140 100644 > > > --- a/fs/nfsd/xdr4.h > > > +++ b/fs/nfsd/xdr4.h > > > @@ -521,6 +521,7 @@ struct nfsd4_copy { > > > u64 cp_src_pos; > > > u64 cp_dst_pos; > > > u64 cp_count; > > > + struct nl4_server *cp_src; > > > > by just a > > > > struct nl4_server cp_src; > > > > since it sounds like you really only need one of them, not a whole array > > (at least for now). > > So this is problematic as the presence of this memory is what is used > to distinguish "inter" from "intra". It would be easy enough to add a new bit for that. > Can things really fail between the xdr and calling of the operation? Yes. Consider a PUTFH+SAVEFH+COPY compound. We decode the whole thing before starting any processing. Then we call nfsd4_putfh() and fh_verify fails (maybe the filehandle is stale or something). Then neither nfsd4_copy() nor nfsd4_encode_copy() will be called. If you absolutely have to do this, you can look at SAVEMEM. --b. > What gets freed in the encoder is the "copy" of the what was decoded > in the decoder. But really freeing in the encoder is the wrong place. > Encoder doesn't need to free. I already free the "copy" of the > copy->cp_src in the cleanup_async_copy(). However, what is missing is > freeing the original copy->cp_src which needs to be freed in the > dup_copy_fields(). > > To clarify: > copy->cp_src gets allocated in the decoder > during the process of the copy: > 1. it gets copied to the kthread and the original copy->cp_src needs > to be freed. Or during any error it will be freed. > 2. cleanup_async_copy frees the copy of the copy->cp_src. > (need to remove the kfree from the encoder). > > > > > --b. > > > > > > > > /* both */ > > > bool cp_synchronous; > > > -- > > > 1.8.3.1
On Fri, Nov 2, 2018 at 12:49 PM J. Bruce Fields <bfields@fieldses.org> wrote: > > On Fri, Nov 02, 2018 at 12:35:26PM -0400, Olga Kornievskaia wrote: > > On Fri, Nov 2, 2018 at 11:46 AM J. Bruce Fields <bfields@fieldses.org> wrote: > > > > > > On Fri, Oct 19, 2018 at 11:28:59AM -0400, Olga Kornievskaia wrote: > > > > @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp, > > > > p = xdr_reserve_space(&resp->xdr, 4 + 4); > > > > *p++ = xdr_one; /* cr_consecutive */ > > > > *p++ = cpu_to_be32(copy->cp_synchronous); > > > > + > > > > + /* allocated in nfsd4_decode_copy */ > > > > + kfree(copy->cp_src); > > > > > > This can result in a leak--for example, if we decode the compound > > > succesfully, but processing fails before we could to this op, then we'll > > > never call this encoder, so we'll allocate without freeing. > > > > > > I think simplest would be to replace this: > > > > > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > > > index feeb6d4..b4d1140 100644 > > > > --- a/fs/nfsd/xdr4.h > > > > +++ b/fs/nfsd/xdr4.h > > > > @@ -521,6 +521,7 @@ struct nfsd4_copy { > > > > u64 cp_src_pos; > > > > u64 cp_dst_pos; > > > > u64 cp_count; > > > > + struct nl4_server *cp_src; > > > > > > by just a > > > > > > struct nl4_server cp_src; > > > > > > since it sounds like you really only need one of them, not a whole array > > > (at least for now). > > > > So this is problematic as the presence of this memory is what is used > > to distinguish "inter" from "intra". > > It would be easy enough to add a new bit for that. > > > Can things really fail between the xdr and calling of the operation? > > Yes. Consider a PUTFH+SAVEFH+COPY compound. We decode the whole thing > before starting any processing. Then we call nfsd4_putfh() and > fh_verify fails (maybe the filehandle is stale or something). Then > neither nfsd4_copy() nor nfsd4_encode_copy() will be called. > > If you absolutely have to do this, you can look at SAVEMEM. Using a new bit is good enough for me. > > --b. > > > What gets freed in the encoder is the "copy" of the what was decoded > > in the decoder. But really freeing in the encoder is the wrong place. > > Encoder doesn't need to free. I already free the "copy" of the > > copy->cp_src in the cleanup_async_copy(). However, what is missing is > > freeing the original copy->cp_src which needs to be freed in the > > dup_copy_fields(). > > > > To clarify: > > copy->cp_src gets allocated in the decoder > > during the process of the copy: > > 1. it gets copied to the kthread and the original copy->cp_src needs > > to be freed. Or during any error it will be freed. > > 2. cleanup_async_copy frees the copy of the copy->cp_src. > > (need to remove the kfree from the encoder). > > > > > > > > --b. > > > > > > > > > > > /* both */ > > > > bool cp_synchronous; > > > > -- > > > > 1.8.3.1
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index 3de42a7..9f6886f 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -40,6 +40,7 @@ #include <linux/utsname.h> #include <linux/pagemap.h> #include <linux/sunrpc/svcauth_gss.h> +#include <linux/sunrpc/addr.h> #include "idmap.h" #include "acl.h" @@ -1743,11 +1744,58 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str DECODE_TAIL; } +static __be32 nfsd4_decode_nl4_server(struct nfsd4_compoundargs *argp, + struct nl4_server *ns) +{ + DECODE_HEAD; + struct nfs42_netaddr *naddr; + + READ_BUF(4); + ns->nl4_type = be32_to_cpup(p++); + + /* currently support for 1 inter-server source server */ + switch (ns->nl4_type) { + case NL4_NAME: + case NL4_URL: + READ_BUF(4); + ns->u.nl4_str_sz = be32_to_cpup(p++); + if (ns->u.nl4_str_sz > NFS4_OPAQUE_LIMIT) + goto xdr_error; + + READ_BUF(ns->u.nl4_str_sz); + COPYMEM(ns->u.nl4_str, + ns->u.nl4_str_sz); + break; + case NL4_NETADDR: + naddr = &ns->u.nl4_addr; + + READ_BUF(4); + naddr->netid_len = be32_to_cpup(p++); + if (naddr->netid_len > RPCBIND_MAXNETIDLEN) + goto xdr_error; + + READ_BUF(naddr->netid_len + 4); /* 4 for uaddr len */ + COPYMEM(naddr->netid, naddr->netid_len); + + naddr->addr_len = be32_to_cpup(p++); + if (naddr->addr_len > RPCBIND_MAXUADDRLEN) + goto xdr_error; + + READ_BUF(naddr->addr_len); + COPYMEM(naddr->addr, naddr->addr_len); + break; + default: + goto xdr_error; + } + DECODE_TAIL; +} + static __be32 nfsd4_decode_copy(struct nfsd4_compoundargs *argp, struct nfsd4_copy *copy) { DECODE_HEAD; - unsigned int tmp; + struct nl4_server *ns; + int i, count; status = nfsd4_decode_stateid(argp, ©->cp_src_stateid); if (status) @@ -1762,8 +1810,24 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str p = xdr_decode_hyper(p, ©->cp_count); p++; /* ca_consecutive: we always do consecutive copies */ copy->cp_synchronous = be32_to_cpup(p++); - tmp = be32_to_cpup(p); /* Source server list not supported */ + count = be32_to_cpup(p++); + + if (count == 0) /* intra-server copy */ + goto intra; + /* decode all the supplied server addresses but use first */ + copy->cp_src = kmalloc(count * sizeof(struct nl4_server), GFP_KERNEL); + if (copy->cp_src == NULL) + return nfserrno(-ENOMEM); + + ns = copy->cp_src; + for (i = 0; i < count; i++) { + status = nfsd4_decode_nl4_server(argp, ns); + if (status) + return status; + ns++; + } +intra: DECODE_TAIL; } @@ -4273,6 +4337,9 @@ static __be32 nfsd4_encode_readv(struct nfsd4_compoundres *resp, p = xdr_reserve_space(&resp->xdr, 4 + 4); *p++ = xdr_one; /* cr_consecutive */ *p++ = cpu_to_be32(copy->cp_synchronous); + + /* allocated in nfsd4_decode_copy */ + kfree(copy->cp_src); return 0; } diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h index feeb6d4..b4d1140 100644 --- a/fs/nfsd/xdr4.h +++ b/fs/nfsd/xdr4.h @@ -521,6 +521,7 @@ struct nfsd4_copy { u64 cp_src_pos; u64 cp_dst_pos; u64 cp_count; + struct nl4_server *cp_src; /* both */ bool cp_synchronous;