| Message ID | 20220111115703.893347-1-hyc.lee@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ksmbd: smbd: fix missing client's memory region invalidation | expand |
2022-01-11 20:57 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > if the Channel of a SMB2 WRITE request is > SMB2_CHANNEL_RDMA_V1_INVALIDTE, a client > does not invalidate its memory regions but > ksmbd must do it by sending a SMB2 WRITE response > with IB_WR_SEND_WITH_INV. > > But if errors occur while processing a SMB2 > READ/WRITE request, ksmbd sends a response > with IB_WR_SEND. So a client could use memory > regions already in use. > > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> > --- > changes from v1: > - factor out setting a remote key to a helper functions. Can we make it as one helper function if desc, Channel, ChannelInfoOffset, ChannelInfoLength are given as arguments? > > fs/ksmbd/smb2pdu.c | 58 +++++++++++++++++++++++++++++++++++----------- > 1 file changed, 44 insertions(+), 14 deletions(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index ced8f949a4d6..b87969576642 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -6124,13 +6124,11 @@ static noinline int smb2_read_pipe(struct ksmbd_work > *work) > return err; > } > > -static ssize_t smb2_read_rdma_channel(struct ksmbd_work *work, > - struct smb2_read_req *req, void *data_buf, > - size_t length) > +static int smb2_set_remote_key_for_rdma_read(struct ksmbd_work *work, > + struct smb2_read_req *req) Can we make it as one helper function if desc, Channel, ChannelInfoOffset, ChannelInfoLength are given as arguments? i.e. smb2_set_remote_key_for_rdma(). > { > struct smb2_buffer_desc_v1 *desc = > (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; > - int err; > > if (work->conn->dialect == SMB30_PROT_ID && > req->Channel != SMB2_CHANNEL_RDMA_V1) > @@ -6143,6 +6141,16 @@ static ssize_t smb2_read_rdma_channel(struct > ksmbd_work *work, > work->need_invalidate_rkey = > (req->Channel == SMB2_CHANNEL_RDMA_V1_INVALIDATE); > work->remote_key = le32_to_cpu(desc->token); > + return 0; > +} > + > +static ssize_t smb2_read_rdma_channel(struct ksmbd_work *work, > + struct smb2_read_req *req, void *data_buf, > + size_t length) > +{ > + struct smb2_buffer_desc_v1 *desc = > + (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; > + int err; > > err = ksmbd_conn_rdma_write(work->conn, data_buf, length, > le32_to_cpu(desc->token), > @@ -6179,6 +6187,13 @@ int smb2_read(struct ksmbd_work *work) > return smb2_read_pipe(work); > } > > + if (req->Channel == SMB2_CHANNEL_RDMA_V1_INVALIDATE || > + req->Channel == SMB2_CHANNEL_RDMA_V1) { > + err = smb2_set_remote_key_for_rdma_read(work, req); > + if (err) > + goto out; > + } > + > fp = ksmbd_lookup_fd_slow(work, le64_to_cpu(req->VolatileFileId), > le64_to_cpu(req->PersistentFileId)); > if (!fp) { > @@ -6352,17 +6367,11 @@ static noinline int smb2_write_pipe(struct > ksmbd_work *work) > return err; > } > > -static ssize_t smb2_write_rdma_channel(struct ksmbd_work *work, > - struct smb2_write_req *req, > - struct ksmbd_file *fp, > - loff_t offset, size_t length, bool sync) > +static int smb2_set_remote_key_for_rdma_write(struct ksmbd_work *work, > + struct smb2_write_req *req) > { > - struct smb2_buffer_desc_v1 *desc; > - char *data_buf; > - int ret; > - ssize_t nbytes; > - > - desc = (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; > + struct smb2_buffer_desc_v1 *desc = > + (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; > > if (work->conn->dialect == SMB30_PROT_ID && > req->Channel != SMB2_CHANNEL_RDMA_V1) > @@ -6378,6 +6387,20 @@ static ssize_t smb2_write_rdma_channel(struct > ksmbd_work *work, > work->need_invalidate_rkey = > (req->Channel == SMB2_CHANNEL_RDMA_V1_INVALIDATE); > work->remote_key = le32_to_cpu(desc->token); > + return 0; > +} > + > +static ssize_t smb2_write_rdma_channel(struct ksmbd_work *work, > + struct smb2_write_req *req, > + struct ksmbd_file *fp, > + loff_t offset, size_t length, bool sync) > +{ > + struct smb2_buffer_desc_v1 *desc; > + char *data_buf; > + int ret; > + ssize_t nbytes; > + > + desc = (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; > > data_buf = kvmalloc(length, GFP_KERNEL | __GFP_ZERO); > if (!data_buf) > @@ -6425,6 +6448,13 @@ int smb2_write(struct ksmbd_work *work) > return smb2_write_pipe(work); > } > > + if (req->Channel == SMB2_CHANNEL_RDMA_V1 || > + req->Channel == SMB2_CHANNEL_RDMA_V1_INVALIDATE) { > + err = smb2_set_remote_key_for_rdma_write(work, req); > + if (err) > + goto out; > + } > + > if (!test_tree_conn_flag(work->tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) { > ksmbd_debug(SMB, "User does not have write permission\n"); > err = -EACCES; > -- > 2.25.1 > >
2022년 1월 11일 (화) 오후 10:21, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > > 2022-01-11 20:57 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > > if the Channel of a SMB2 WRITE request is > > SMB2_CHANNEL_RDMA_V1_INVALIDTE, a client > > does not invalidate its memory regions but > > ksmbd must do it by sending a SMB2 WRITE response > > with IB_WR_SEND_WITH_INV. > > > > But if errors occur while processing a SMB2 > > READ/WRITE request, ksmbd sends a response > > with IB_WR_SEND. So a client could use memory > > regions already in use. > > > > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> > > --- > > changes from v1: > > - factor out setting a remote key to a helper functions. > Can we make it as one helper function if desc, Channel, > ChannelInfoOffset, ChannelInfoLength are given as arguments? > Okay, I will change it. > > > > fs/ksmbd/smb2pdu.c | 58 +++++++++++++++++++++++++++++++++++----------- > > 1 file changed, 44 insertions(+), 14 deletions(-) > > > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > > index ced8f949a4d6..b87969576642 100644 > > --- a/fs/ksmbd/smb2pdu.c > > +++ b/fs/ksmbd/smb2pdu.c > > @@ -6124,13 +6124,11 @@ static noinline int smb2_read_pipe(struct ksmbd_work > > *work) > > return err; > > } > > > > -static ssize_t smb2_read_rdma_channel(struct ksmbd_work *work, > > - struct smb2_read_req *req, void *data_buf, > > - size_t length) > > +static int smb2_set_remote_key_for_rdma_read(struct ksmbd_work *work, > > + struct smb2_read_req *req) > Can we make it as one helper function if desc, Channel, > ChannelInfoOffset, ChannelInfoLength are given as arguments? i.e. > smb2_set_remote_key_for_rdma(). > > > { > > struct smb2_buffer_desc_v1 *desc = > > (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; > > - int err; > > > > if (work->conn->dialect == SMB30_PROT_ID && > > req->Channel != SMB2_CHANNEL_RDMA_V1) > > @@ -6143,6 +6141,16 @@ static ssize_t smb2_read_rdma_channel(struct > > ksmbd_work *work, > > work->need_invalidate_rkey = > > (req->Channel == SMB2_CHANNEL_RDMA_V1_INVALIDATE); > > work->remote_key = le32_to_cpu(desc->token); > > + return 0; > > +} > > + > > +static ssize_t smb2_read_rdma_channel(struct ksmbd_work *work, > > + struct smb2_read_req *req, void *data_buf, > > + size_t length) > > +{ > > + struct smb2_buffer_desc_v1 *desc = > > + (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; > > + int err; > > > > err = ksmbd_conn_rdma_write(work->conn, data_buf, length, > > le32_to_cpu(desc->token), > > @@ -6179,6 +6187,13 @@ int smb2_read(struct ksmbd_work *work) > > return smb2_read_pipe(work); > > } > > > > + if (req->Channel == SMB2_CHANNEL_RDMA_V1_INVALIDATE || > > + req->Channel == SMB2_CHANNEL_RDMA_V1) { > > + err = smb2_set_remote_key_for_rdma_read(work, req); > > + if (err) > > + goto out; > > + } > > + > > fp = ksmbd_lookup_fd_slow(work, le64_to_cpu(req->VolatileFileId), > > le64_to_cpu(req->PersistentFileId)); > > if (!fp) { > > @@ -6352,17 +6367,11 @@ static noinline int smb2_write_pipe(struct > > ksmbd_work *work) > > return err; > > } > > > > -static ssize_t smb2_write_rdma_channel(struct ksmbd_work *work, > > - struct smb2_write_req *req, > > - struct ksmbd_file *fp, > > - loff_t offset, size_t length, bool sync) > > +static int smb2_set_remote_key_for_rdma_write(struct ksmbd_work *work, > > + struct smb2_write_req *req) > > { > > - struct smb2_buffer_desc_v1 *desc; > > - char *data_buf; > > - int ret; > > - ssize_t nbytes; > > - > > - desc = (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; > > + struct smb2_buffer_desc_v1 *desc = > > + (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; > > > > if (work->conn->dialect == SMB30_PROT_ID && > > req->Channel != SMB2_CHANNEL_RDMA_V1) > > @@ -6378,6 +6387,20 @@ static ssize_t smb2_write_rdma_channel(struct > > ksmbd_work *work, > > work->need_invalidate_rkey = > > (req->Channel == SMB2_CHANNEL_RDMA_V1_INVALIDATE); > > work->remote_key = le32_to_cpu(desc->token); > > + return 0; > > +} > > + > > +static ssize_t smb2_write_rdma_channel(struct ksmbd_work *work, > > + struct smb2_write_req *req, > > + struct ksmbd_file *fp, > > + loff_t offset, size_t length, bool sync) > > +{ > > + struct smb2_buffer_desc_v1 *desc; > > + char *data_buf; > > + int ret; > > + ssize_t nbytes; > > + > > + desc = (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; > > > > data_buf = kvmalloc(length, GFP_KERNEL | __GFP_ZERO); > > if (!data_buf) > > @@ -6425,6 +6448,13 @@ int smb2_write(struct ksmbd_work *work) > > return smb2_write_pipe(work); > > } > > > > + if (req->Channel == SMB2_CHANNEL_RDMA_V1 || > > + req->Channel == SMB2_CHANNEL_RDMA_V1_INVALIDATE) { > > + err = smb2_set_remote_key_for_rdma_write(work, req); > > + if (err) > > + goto out; > > + } > > + > > if (!test_tree_conn_flag(work->tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) { > > ksmbd_debug(SMB, "User does not have write permission\n"); > > err = -EACCES; > > -- > > 2.25.1 > > > >
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index ced8f949a4d6..b87969576642 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -6124,13 +6124,11 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work) return err; } -static ssize_t smb2_read_rdma_channel(struct ksmbd_work *work, - struct smb2_read_req *req, void *data_buf, - size_t length) +static int smb2_set_remote_key_for_rdma_read(struct ksmbd_work *work, + struct smb2_read_req *req) { struct smb2_buffer_desc_v1 *desc = (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; - int err; if (work->conn->dialect == SMB30_PROT_ID && req->Channel != SMB2_CHANNEL_RDMA_V1) @@ -6143,6 +6141,16 @@ static ssize_t smb2_read_rdma_channel(struct ksmbd_work *work, work->need_invalidate_rkey = (req->Channel == SMB2_CHANNEL_RDMA_V1_INVALIDATE); work->remote_key = le32_to_cpu(desc->token); + return 0; +} + +static ssize_t smb2_read_rdma_channel(struct ksmbd_work *work, + struct smb2_read_req *req, void *data_buf, + size_t length) +{ + struct smb2_buffer_desc_v1 *desc = + (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; + int err; err = ksmbd_conn_rdma_write(work->conn, data_buf, length, le32_to_cpu(desc->token), @@ -6179,6 +6187,13 @@ int smb2_read(struct ksmbd_work *work) return smb2_read_pipe(work); } + if (req->Channel == SMB2_CHANNEL_RDMA_V1_INVALIDATE || + req->Channel == SMB2_CHANNEL_RDMA_V1) { + err = smb2_set_remote_key_for_rdma_read(work, req); + if (err) + goto out; + } + fp = ksmbd_lookup_fd_slow(work, le64_to_cpu(req->VolatileFileId), le64_to_cpu(req->PersistentFileId)); if (!fp) { @@ -6352,17 +6367,11 @@ static noinline int smb2_write_pipe(struct ksmbd_work *work) return err; } -static ssize_t smb2_write_rdma_channel(struct ksmbd_work *work, - struct smb2_write_req *req, - struct ksmbd_file *fp, - loff_t offset, size_t length, bool sync) +static int smb2_set_remote_key_for_rdma_write(struct ksmbd_work *work, + struct smb2_write_req *req) { - struct smb2_buffer_desc_v1 *desc; - char *data_buf; - int ret; - ssize_t nbytes; - - desc = (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; + struct smb2_buffer_desc_v1 *desc = + (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; if (work->conn->dialect == SMB30_PROT_ID && req->Channel != SMB2_CHANNEL_RDMA_V1) @@ -6378,6 +6387,20 @@ static ssize_t smb2_write_rdma_channel(struct ksmbd_work *work, work->need_invalidate_rkey = (req->Channel == SMB2_CHANNEL_RDMA_V1_INVALIDATE); work->remote_key = le32_to_cpu(desc->token); + return 0; +} + +static ssize_t smb2_write_rdma_channel(struct ksmbd_work *work, + struct smb2_write_req *req, + struct ksmbd_file *fp, + loff_t offset, size_t length, bool sync) +{ + struct smb2_buffer_desc_v1 *desc; + char *data_buf; + int ret; + ssize_t nbytes; + + desc = (struct smb2_buffer_desc_v1 *)&req->Buffer[0]; data_buf = kvmalloc(length, GFP_KERNEL | __GFP_ZERO); if (!data_buf) @@ -6425,6 +6448,13 @@ int smb2_write(struct ksmbd_work *work) return smb2_write_pipe(work); } + if (req->Channel == SMB2_CHANNEL_RDMA_V1 || + req->Channel == SMB2_CHANNEL_RDMA_V1_INVALIDATE) { + err = smb2_set_remote_key_for_rdma_write(work, req); + if (err) + goto out; + } + if (!test_tree_conn_flag(work->tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) { ksmbd_debug(SMB, "User does not have write permission\n"); err = -EACCES;
if the Channel of a SMB2 WRITE request is SMB2_CHANNEL_RDMA_V1_INVALIDTE, a client does not invalidate its memory regions but ksmbd must do it by sending a SMB2 WRITE response with IB_WR_SEND_WITH_INV. But if errors occur while processing a SMB2 READ/WRITE request, ksmbd sends a response with IB_WR_SEND. So a client could use memory regions already in use. Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> --- changes from v1: - factor out setting a remote key to a helper functions. fs/ksmbd/smb2pdu.c | 58 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 14 deletions(-)