On Fri, Sep 8, 2017 at 3:38 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> Some questions below, but really I think this approach can't work. Keep
> in mind this has to work not just for the one compound the current
> client uses, but for *any* compound containing a COPY, including those
> sent by future clients that may do something more complicated, or by
> malcious clients that may intentionally send weird compounds to make the
> server crash.
> I think we want to flag the filehandle itself somehow, not the state.
> And check very carefully to make sure that ops other than COPY can't get
> tripped up by one of these foreign filehandles.
>
> On Tue, Jul 11, 2017 at 12:44:01PM -0400, Olga Kornievskaia wrote:
>> The inter server to server COPY source server filehandle
>> is guaranteed to be stale as the COPY is sent to the destination
>> server.
>>
>> Signed-off-by: Andy Adamson <andros@xxxxxxxxxx>
>> ---
>> fs/nfsd/nfs4proc.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>> fs/nfsd/nfs4xdr.c | 26 +++++++++++++++++++++++++-
>> fs/nfsd/nfsd.h | 2 ++
>> fs/nfsd/xdr4.h | 4 ++++
>> 4 files changed, 77 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index b49ff31..ceee852 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -496,11 +496,19 @@ static __be32 nfsd4_open_omfg(struct svc_rqst *rqstp, struct nfsd4_compound_stat
>> nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> struct nfsd4_putfh *putfh)
>> {
>> + __be32 ret;
>> +
>> fh_put(&cstate->current_fh);
>> cstate->current_fh.fh_handle.fh_size = putfh->pf_fhlen;
>> memcpy(&cstate->current_fh.fh_handle.fh_base, putfh->pf_fhval,
>> putfh->pf_fhlen);
>> - return fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
>> + ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
>> + if (ret == nfserr_stale && HAS_CSTATE_FLAG(cstate, NO_VERIFY_FH)) {
>> + CLEAR_CSTATE_FLAG(cstate, NO_VERIFY_FH);
>> + SET_CSTATE_FLAG(cstate, IS_STALE_FH);
>> + ret = 0;
>> + }
>> + return ret;
>> }
>>
>> static __be32
>> @@ -533,6 +541,16 @@ static __be32 nfsd4_open_omfg(struct svc_rqst *rqstp, struct nfsd4_compound_stat
>> nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> void *arg)
>> {
>> + /**
>> + * This is either an inter COPY (most likely) or an intra COPY with a
>> + * stale file handle. If the latter, nfsd4_copy will reset the PUTFH to
>> + * return nfserr_stale. No fh_dentry, just copy the file handle
>> + * to use with the inter COPY READ.
>> + */
>> + if (HAS_CSTATE_FLAG(cstate, IS_STALE_FH)) {
>> + cstate->save_fh = cstate->current_fh;
>> + return nfs_ok;
>> + }
>> if (!cstate->current_fh.fh_dentry)
>> return nfserr_nofilehandle;
>>
>> @@ -1067,6 +1085,13 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>> if (status)
>> goto out;
>>
>> + /* Intra copy source fh is stale. PUTFH will fail with ESTALE */
>> + if (HAS_CSTATE_FLAG(cstate, IS_STALE_FH)) {
>> + CLEAR_CSTATE_FLAG(cstate, IS_STALE_FH);
>> + cstate->status = nfserr_copy_stalefh;
>> + goto out_put;
>> + }
>> +
>> bytes = nfsd_copy_file_range(src, copy->cp_src_pos,
>> dst, copy->cp_dst_pos, copy->cp_count);
>>
>> @@ -1081,6 +1106,7 @@ static int fill_in_write_vector(struct kvec *vec, struct nfsd4_write *write)
>> status = nfs_ok;
>> }
>>
>> +out_put:
>> fput(src);
>> fput(dst);
>> out:
>> @@ -1759,6 +1785,7 @@ static void svcxdr_init_encode(struct svc_rqst *rqstp,
>> struct nfsd4_compound_state *cstate = &resp->cstate;
>> struct svc_fh *current_fh = &cstate->current_fh;
>> struct svc_fh *save_fh = &cstate->save_fh;
>> + int i;
>> __be32 status;
>>
>> svcxdr_init_encode(rqstp, resp);
>> @@ -1791,6 +1818,12 @@ static void svcxdr_init_encode(struct svc_rqst *rqstp,
>> goto encode_op;
>> }
>>
>> + /* NFSv4.2 COPY source file handle may be from a different server */
>> + for (i = 0; i < args->opcnt; i++) {
>> + op = &args->ops[i];
>> + if (op->opnum == OP_COPY)
>> + SET_CSTATE_FLAG(cstate, NO_VERIFY_FH);
>> + }
>
> So, if a compound has a COPY anywhere in it, then a PUTFH with a stale
> filehandle anywhere in the compound will temporarily succeed and result in
> IS_STALE_FH being set.
How about changing it to be more restrictive by checking within the
while loop that
if it's a PUTFH and it's followed by the SAVE_FH+PUTFH+COPY only
then set the NO_VERIFY_FH.
while (!status && resp->opcnt < args->opcnt) {
op = &args->ops[resp->opcnt++];
if (op->opnum == OP_PUTFH &&
args->ops[resp->opcnt].opnum == OP_SAVEFH &&
args->ops[resp->opcnt+1].opnum == OP_PUTFH &&
args->ops[resp->opcnt+2].opnum == OP_COPY) {
SET_CSTATE_FLAG(cstate, NO_VERIFY_FH);
}
Would that address your concern or is this too restrictive?
I guess we can allow some other operations between the 2nd PUTFH and COPY
because the 2nd filehandle will be validated and must be valid to continue
processing the compound. It doesn't make sense to allow for operation after the
1st PUTFH as it can be a foreign handle.
Somewhere else you were talking about how a "foreign" file handle can mean
something to the server. If that's the case and we do allow for operations
between putfh, savefh, putfh then we'll get into trouble that I can't
think we can
get out of.
If it's a inter copy and the source file handle means something to the
server I can
think of the following scenario: the state won't be flagged IS_STALE then the
filehandle would go thru the checks you listed below and can (unintentionally)
result in an error (eg., err_moved?).
To really handle this, i can see doing something like, for a putfh with a copy
elsewhere in the compound, skip ahead to the copy, find out if it's an
inter copy,
then we know to mark the filehandle (as you suggested in the beginning) and
check in all operations that they are not being done on a foreign handle. That
seems very messy... Isn't restricting to putfh, savefh, putfh, copy a better way
for the current situation?
>
>> while (!status && resp->opcnt < args->opcnt) {
>> op = &args->ops[resp->opcnt++];
>>
>> @@ -1810,6 +1843,9 @@ static void svcxdr_init_encode(struct svc_rqst *rqstp,
>>
>> opdesc = OPDESC(op);
>>
>> + if (HAS_CSTATE_FLAG(cstate, IS_STALE_FH))
>> + goto call_op;
>> +
>
> That means that once IS_STALE_FH is set, we will skip:
>
> - nofilehandle checking
> - moved checking
> - fh_clear_wcc()
> - resp_size checking
> - current stateid capture
>
> on every subsequent op. Can that really be right?
This should be changed to
if (HAS_CSTATE_FLAG(cstate, IS_STALE_FH) && op->opnum == OP_SAVEFH)
then we need to skip the checks for savefh as it has no valid file handle.
Does that address your concern?
>
>> if (!current_fh->fh_dentry) {
>> if (!(opdesc->op_flags & ALLOWED_WITHOUT_FH)) {
>> op->status = nfserr_nofilehandle;
>> @@ -1844,6 +1880,7 @@ static void svcxdr_init_encode(struct svc_rqst *rqstp,
>>
>> if (opdesc->op_get_currentstateid)
>> opdesc->op_get_currentstateid(cstate, &op->u);
>> +call_op:
>> op->status = opdesc->op_func(rqstp, cstate, &op->u);
>>
>> /* Only from SEQUENCE */
>> @@ -1862,6 +1899,14 @@ static void svcxdr_init_encode(struct svc_rqst *rqstp,
>> if (need_wrongsec_check(rqstp))
>> op->status = check_nfsd_access(current_fh->fh_export, rqstp);
>> }
>> + /* Only from intra COPY */
>> + if (cstate->status == nfserr_copy_stalefh) {
>> + dprintk("%s NFS4.2 intra COPY stale src filehandle\n",
>> + __func__);
>> + status = nfserr_stale;
>> + nfsd4_adjust_encode(resp);
>
> Are you sure it's safe just to throw away any operations since that
> stale PUTFH? What if some of those operations had side effects?
If COPY comes in PUTFH,SAVEFH, PUTFH,COPY compound then
I think it's safe?
>
>> + goto out;
>> + }
>> encode_op:
>> if (op->status == nfserr_replay_me) {
>> op->replay = &cstate->replay_owner->so_replay;
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 3e08c15..2896a11 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -4624,15 +4624,28 @@ __be32 nfsd4_check_resp_size(struct nfsd4_compoundres *resp, u32 respsize)
>> return nfserr_rep_too_big;
>> }
>>
>> +/** Rewind the encoding to return nfserr_stale on the PUTFH
>> + * in this failed Intra COPY compound
>> + */
>> +void
>> +nfsd4_adjust_encode(struct nfsd4_compoundres *resp)
>> +{
>> + __be32 *p;
>> +
>> + p = resp->cstate.putfh_errp;
>> + *p++ = nfserr_stale;
>> +}
>> +
>> void
>> nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
>> {
>> struct xdr_stream *xdr = &resp->xdr;
>> struct nfs4_stateowner *so = resp->cstate.replay_owner;
>> + struct nfsd4_compound_state *cstate = &resp->cstate;
>> struct svc_rqst *rqstp = resp->rqstp;
>> int post_err_offset;
>> nfsd4_enc encoder;
>> - __be32 *p;
>> + __be32 *p, *statp;
>>
>> p = xdr_reserve_space(xdr, 8);
>> if (!p) {
>> @@ -4641,9 +4654,20 @@ __be32 nfsd4_check_resp_size(struct nfsd4_compoundres *resp, u32 respsize)
>> }
>> *p++ = cpu_to_be32(op->opnum);
>> post_err_offset = xdr->buf->len;
>> + statp = p;
>>
>> if (op->opnum == OP_ILLEGAL)
>> goto status;
>> +
>> + /** This is a COPY compound with a stale source server file handle.
>> + * If OP_COPY processing determines that this is an intra server to
>> + * server COPY, then this PUTFH should return nfserr_ stale so the
>> + * putfh_errp will be set to nfserr_stale. If this is an inter server
>> + * to server COPY, ignore the nfserr_stale.
>> + */
>> + if (op->opnum == OP_PUTFH && HAS_CSTATE_FLAG(cstate, IS_STALE_FH))
>> + cstate->putfh_errp = statp;
>> +
>> BUG_ON(op->opnum < 0 || op->opnum >= ARRAY_SIZE(nfsd4_enc_ops) ||
>> !nfsd4_enc_ops[op->opnum]);
>> encoder = nfsd4_enc_ops[op->opnum];
>> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
>> index d966068..8d6fb0f 100644
>> --- a/fs/nfsd/nfsd.h
>> +++ b/fs/nfsd/nfsd.h
>> @@ -272,6 +272,8 @@ static inline bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
>> #define nfserr_replay_me cpu_to_be32(11001)
>> /* nfs41 replay detected */
>> #define nfserr_replay_cache cpu_to_be32(11002)
>> +/* nfs42 intra copy failed with nfserr_stale */
>> +#define nfserr_copy_stalefh cpu_to_be32(1103)
>>
>> /* Check for dir entries '.' and '..' */
>> #define isdotent(n, l) (l < 3 && n[0] == '.' && (l == 1 || n[1] == '.'))
>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>> index 38fcb4f..aa94295 100644
>> --- a/fs/nfsd/xdr4.h
>> +++ b/fs/nfsd/xdr4.h
>> @@ -45,6 +45,8 @@
>>
>> #define CURRENT_STATE_ID_FLAG (1<<0)
>> #define SAVED_STATE_ID_FLAG (1<<1)
>> +#define NO_VERIFY_FH (1<<2)
>> +#define IS_STALE_FH (1<<3)
>>
>> #define SET_CSTATE_FLAG(c, f) ((c)->sid_flags |= (f))
>> #define HAS_CSTATE_FLAG(c, f) ((c)->sid_flags & (f))
>> @@ -63,6 +65,7 @@ struct nfsd4_compound_state {
>> size_t iovlen;
>> u32 minorversion;
>> __be32 status;
>> + __be32 *putfh_errp;
>> stateid_t current_stateid;
>> stateid_t save_stateid;
>> /* to indicate current and saved state id presents */
>> @@ -705,6 +708,7 @@ int nfs4svc_decode_compoundargs(struct svc_rqst *, __be32 *,
>> int nfs4svc_encode_compoundres(struct svc_rqst *, __be32 *,
>> struct nfsd4_compoundres *);
>> __be32 nfsd4_check_resp_size(struct nfsd4_compoundres *, u32);
>> +void nfsd4_adjust_encode(struct nfsd4_compoundres *);
>> void nfsd4_encode_operation(struct nfsd4_compoundres *, struct nfsd4_op *);
>> void nfsd4_encode_replay(struct xdr_stream *xdr, struct nfsd4_op *op);
>> __be32 nfsd4_encode_fattr_to_buf(__be32 **p, int words,
>> --
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
