Re: [PATCH] cifs: remove useless parameter 'is_fsctl' from SMB2_ioctl()

[Tom Talpey <tom@xxxxxxxxxx>]

Sending that bit violates the protocol. So if you are keeping
it in reserve, what protocol extension will use it? And, will
it be documented like MS-SMB2??

I'm with Enzo IOW.

Tom.

On 8/17/2022 4:14 PM, Steve French wrote:
> Let's think about this one more, maybe try some experiments at the
> upcoming plugfest with other servers.
>
> There is a small possibility that there may be debug workloads that
> supported this on some servers, and no hurry to remove this parm.
>
> On Wed, Aug 17, 2022 at 2:59 PM Enzo Matsumiya <ematsumiya@xxxxxxx> wrote:
> > On 08/17, Steve French wrote:
> > > One alternative would be to allow the user space "pass through ioctl"
> > > to set this flag to false (in case there were cases where a server did
> > > support it and a test program or userspace utility needs to set it).
> >
> > I don't really see the point of having so.
> >
> > > Do both Samba and ksmbd always reject it if isFsctl is false?
> >
> > Yes.
> >
> > For reference, Samba in smbd_smb2_request_ioctl_done() (source3/smbd/smb2_ioctl.c):
> >
> > 7599         if (req->Flags != cpu_to_le32(SMB2_0_IOCTL_IS_FSCTL)) {
> > 7600                 rsp->hdr.Status = STATUS_NOT_SUPPORTED;
> > 7601                 goto out;
> > 7602         }
> >
> > and in ksmbd smb2_ioctl() (fs/ksmbd/smb2pdu.c):
> >
> > 7599         if (req->Flags != cpu_to_le32(SMB2_0_IOCTL_IS_FSCTL)) {
> > 7600                 rsp->hdr.Status = STATUS_NOT_SUPPORTED;
> > 7601                 goto out;
> > 7602         }
> >
> >
> > Cheers,
> >
> > Enzo
> >
> > > On Wed, Aug 17, 2022 at 2:08 PM Enzo Matsumiya <ematsumiya@xxxxxxx> wrote:
> > > > SMB2_ioctl() is always called with is_fsctl = true, so doesn't make any
> > > > sense to have it at all.
> > > >
> > > > Thus, always set SMB2_0_IOCTL_IS_FSCTL flag on the request.
> > > >
> > > > Also, as per MS-SMB2 3.3.5.15 "Receiving an SMB2 IOCTL Request", servers
> > > > must fail the request if the request flags is zero anyway.
> > > >
> > > > Signed-off-by: Enzo Matsumiya <ematsumiya@xxxxxxx>
> > > > ---
> > > >   fs/cifs/smb2file.c  |  1 -
> > > >   fs/cifs/smb2ops.c   | 35 +++++++++++++----------------------
> > > >   fs/cifs/smb2pdu.c   | 20 +++++++++-----------
> > > >   fs/cifs/smb2proto.h |  4 ++--
> > > >   4 files changed, 24 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/fs/cifs/smb2file.c b/fs/cifs/smb2file.c
> > > > index f5dcc4940b6d..9dfd2dd612c2 100644
> > > > --- a/fs/cifs/smb2file.c
> > > > +++ b/fs/cifs/smb2file.c
> > > > @@ -61,7 +61,6 @@ smb2_open_file(const unsigned int xid, struct cifs_open_parms *oparms,
> > > >                  nr_ioctl_req.Reserved = 0;
> > > >                  rc = SMB2_ioctl(xid, oparms->tcon, fid->persistent_fid,
> > > >                          fid->volatile_fid, FSCTL_LMR_REQUEST_RESILIENCY,
> > > > -                       true /* is_fsctl */,
> > > >                          (char *)&nr_ioctl_req, sizeof(nr_ioctl_req),
> > > >                          CIFSMaxBufSize, NULL, NULL /* no return info */);
> > > >                  if (rc == -EOPNOTSUPP) {
> > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > > index f406af596887..c8ada000de7f 100644
> > > > --- a/fs/cifs/smb2ops.c
> > > > +++ b/fs/cifs/smb2ops.c
> > > > @@ -681,7 +681,7 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon)
> > > >          struct cifs_ses *ses = tcon->ses;
> > > >
> > > >          rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> > > > -                       FSCTL_QUERY_NETWORK_INTERFACE_INFO, true /* is_fsctl */,
> > > > +                       FSCTL_QUERY_NETWORK_INTERFACE_INFO,
> > > >                          NULL /* no data input */, 0 /* no data input */,
> > > >                          CIFSMaxBufSize, (char **)&out_buf, &ret_data_len);
> > > >          if (rc == -EOPNOTSUPP) {
> > > > @@ -1323,9 +1323,8 @@ SMB2_request_res_key(const unsigned int xid, struct cifs_tcon *tcon,
> > > >          struct resume_key_req *res_key;
> > > >
> > > >          rc = SMB2_ioctl(xid, tcon, persistent_fid, volatile_fid,
> > > > -                       FSCTL_SRV_REQUEST_RESUME_KEY, true /* is_fsctl */,
> > > > -                       NULL, 0 /* no input */, CIFSMaxBufSize,
> > > > -                       (char **)&res_key, &ret_data_len);
> > > > +                       FSCTL_SRV_REQUEST_RESUME_KEY, NULL, 0 /* no input */,
> > > > +                       CIFSMaxBufSize, (char **)&res_key, &ret_data_len);
> > > >
> > > >          if (rc == -EOPNOTSUPP) {
> > > >                  pr_warn_once("Server share %s does not support copy range\n", tcon->treeName);
> > > > @@ -1467,7 +1466,7 @@ smb2_ioctl_query_info(const unsigned int xid,
> > > >                  rqst[1].rq_nvec = SMB2_IOCTL_IOV_SIZE;
> > > >
> > > >                  rc = SMB2_ioctl_init(tcon, server, &rqst[1], COMPOUND_FID, COMPOUND_FID,
> > > > -                                    qi.info_type, true, buffer, qi.output_buffer_length,
> > > > +                                    qi.info_type, buffer, qi.output_buffer_length,
> > > >                                       CIFSMaxBufSize - MAX_SMB2_CREATE_RESPONSE_SIZE -
> > > >                                       MAX_SMB2_CLOSE_RESPONSE_SIZE);
> > > >                  free_req1_func = SMB2_ioctl_free;
> > > > @@ -1643,9 +1642,8 @@ smb2_copychunk_range(const unsigned int xid,
> > > >                  retbuf = NULL;
> > > >                  rc = SMB2_ioctl(xid, tcon, trgtfile->fid.persistent_fid,
> > > >                          trgtfile->fid.volatile_fid, FSCTL_SRV_COPYCHUNK_WRITE,
> > > > -                       true /* is_fsctl */, (char *)pcchunk,
> > > > -                       sizeof(struct copychunk_ioctl), CIFSMaxBufSize,
> > > > -                       (char **)&retbuf, &ret_data_len);
> > > > +                       (char *)pcchunk, sizeof(struct copychunk_ioctl),
> > > > +                       CIFSMaxBufSize, (char **)&retbuf, &ret_data_len);
> > > >                  if (rc == 0) {
> > > >                          if (ret_data_len !=
> > > >                                          sizeof(struct copychunk_ioctl_rsp)) {
> > > > @@ -1805,7 +1803,6 @@ static bool smb2_set_sparse(const unsigned int xid, struct cifs_tcon *tcon,
> > > >
> > > >          rc = SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
> > > >                          cfile->fid.volatile_fid, FSCTL_SET_SPARSE,
> > > > -                       true /* is_fctl */,
> > > >                          &setsparse, 1, CIFSMaxBufSize, NULL, NULL);
> > > >          if (rc) {
> > > >                  tcon->broken_sparse_sup = true;
> > > > @@ -1888,7 +1885,6 @@ smb2_duplicate_extents(const unsigned int xid,
> > > >          rc = SMB2_ioctl(xid, tcon, trgtfile->fid.persistent_fid,
> > > >                          trgtfile->fid.volatile_fid,
> > > >                          FSCTL_DUPLICATE_EXTENTS_TO_FILE,
> > > > -                       true /* is_fsctl */,
> > > >                          (char *)&dup_ext_buf,
> > > >                          sizeof(struct duplicate_extents_to_file),
> > > >                          CIFSMaxBufSize, NULL,
> > > > @@ -1923,7 +1919,6 @@ smb3_set_integrity(const unsigned int xid, struct cifs_tcon *tcon,
> > > >          return SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
> > > >                          cfile->fid.volatile_fid,
> > > >                          FSCTL_SET_INTEGRITY_INFORMATION,
> > > > -                       true /* is_fsctl */,
> > > >                          (char *)&integr_info,
> > > >                          sizeof(struct fsctl_set_integrity_information_req),
> > > >                          CIFSMaxBufSize, NULL,
> > > > @@ -1976,7 +1971,6 @@ smb3_enum_snapshots(const unsigned int xid, struct cifs_tcon *tcon,
> > > >          rc = SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
> > > >                          cfile->fid.volatile_fid,
> > > >                          FSCTL_SRV_ENUMERATE_SNAPSHOTS,
> > > > -                       true /* is_fsctl */,
> > > >                          NULL, 0 /* no input data */, max_response_size,
> > > >                          (char **)&retbuf,
> > > >                          &ret_data_len);
> > > > @@ -2699,7 +2693,6 @@ smb2_get_dfs_refer(const unsigned int xid, struct cifs_ses *ses,
> > > >          do {
> > > >                  rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> > > >                                  FSCTL_DFS_GET_REFERRALS,
> > > > -                               true /* is_fsctl */,
> > > >                                  (char *)dfs_req, dfs_req_size, CIFSMaxBufSize,
> > > >                                  (char **)&dfs_rsp, &dfs_rsp_size);
> > > >                  if (!is_retryable_error(rc))
> > > > @@ -2906,8 +2899,7 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon,
> > > >
> > > >          rc = SMB2_ioctl_init(tcon, server,
> > > >                               &rqst[1], fid.persistent_fid,
> > > > -                            fid.volatile_fid, FSCTL_GET_REPARSE_POINT,
> > > > -                            true /* is_fctl */, NULL, 0,
> > > > +                            fid.volatile_fid, FSCTL_GET_REPARSE_POINT, NULL, 0,
> > > >                               CIFSMaxBufSize -
> > > >                               MAX_SMB2_CREATE_RESPONSE_SIZE -
> > > >                               MAX_SMB2_CLOSE_RESPONSE_SIZE);
> > > > @@ -3087,8 +3079,7 @@ smb2_query_reparse_tag(const unsigned int xid, struct cifs_tcon *tcon,
> > > >
> > > >          rc = SMB2_ioctl_init(tcon, server,
> > > >                               &rqst[1], COMPOUND_FID,
> > > > -                            COMPOUND_FID, FSCTL_GET_REPARSE_POINT,
> > > > -                            true /* is_fctl */, NULL, 0,
> > > > +                            COMPOUND_FID, FSCTL_GET_REPARSE_POINT, NULL, 0,
> > > >                               CIFSMaxBufSize -
> > > >                               MAX_SMB2_CREATE_RESPONSE_SIZE -
> > > >                               MAX_SMB2_CLOSE_RESPONSE_SIZE);
> > > > @@ -3358,7 +3349,7 @@ static long smb3_zero_range(struct file *file, struct cifs_tcon *tcon,
> > > >          fsctl_buf.BeyondFinalZero = cpu_to_le64(offset + len);
> > > >
> > > >          rc = SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
> > > > -                       cfile->fid.volatile_fid, FSCTL_SET_ZERO_DATA, true,
> > > > +                       cfile->fid.volatile_fid, FSCTL_SET_ZERO_DATA,
> > > >                          (char *)&fsctl_buf,
> > > >                          sizeof(struct file_zero_data_information),
> > > >                          0, NULL, NULL);
> > > > @@ -3421,7 +3412,7 @@ static long smb3_punch_hole(struct file *file, struct cifs_tcon *tcon,
> > > >
> > > >          rc = SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
> > > >                          cfile->fid.volatile_fid, FSCTL_SET_ZERO_DATA,
> > > > -                       true /* is_fctl */, (char *)&fsctl_buf,
> > > > +                       (char *)&fsctl_buf,
> > > >                          sizeof(struct file_zero_data_information),
> > > >                          CIFSMaxBufSize, NULL, NULL);
> > > >          free_xid(xid);
> > > > @@ -3481,7 +3472,7 @@ static int smb3_simple_fallocate_range(unsigned int xid,
> > > >          in_data.length = cpu_to_le64(len);
> > > >          rc = SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
> > > >                          cfile->fid.volatile_fid,
> > > > -                       FSCTL_QUERY_ALLOCATED_RANGES, true,
> > > > +                       FSCTL_QUERY_ALLOCATED_RANGES,
> > > >                          (char *)&in_data, sizeof(in_data),
> > > >                          1024 * sizeof(struct file_allocated_range_buffer),
> > > >                          (char **)&out_data, &out_data_len);
> > > > @@ -3802,7 +3793,7 @@ static loff_t smb3_llseek(struct file *file, struct cifs_tcon *tcon, loff_t offs
> > > >
> > > >          rc = SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
> > > >                          cfile->fid.volatile_fid,
> > > > -                       FSCTL_QUERY_ALLOCATED_RANGES, true,
> > > > +                       FSCTL_QUERY_ALLOCATED_RANGES,
> > > >                          (char *)&in_data, sizeof(in_data),
> > > >                          sizeof(struct file_allocated_range_buffer),
> > > >                          (char **)&out_data, &out_data_len);
> > > > @@ -3862,7 +3853,7 @@ static int smb3_fiemap(struct cifs_tcon *tcon,
> > > >
> > > >          rc = SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
> > > >                          cfile->fid.volatile_fid,
> > > > -                       FSCTL_QUERY_ALLOCATED_RANGES, true,
> > > > +                       FSCTL_QUERY_ALLOCATED_RANGES,
> > > >                          (char *)&in_data, sizeof(in_data),
> > > >                          1024 * sizeof(struct file_allocated_range_buffer),
> > > >                          (char **)&out_data, &out_data_len);
> > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > > index 9b31ea946d45..918152fb8582 100644
> > > > --- a/fs/cifs/smb2pdu.c
> > > > +++ b/fs/cifs/smb2pdu.c
> > > > @@ -1173,7 +1173,7 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
> > > >          }
> > > >
> > > >          rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> > > > -               FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
> > > > +               FSCTL_VALIDATE_NEGOTIATE_INFO,
> > > >                  (char *)pneg_inbuf, inbuflen, CIFSMaxBufSize,
> > > >                  (char **)&pneg_rsp, &rsplen);
> > > >          if (rc == -EOPNOTSUPP) {
> > > > @@ -3056,7 +3056,7 @@ int
> > > >   SMB2_ioctl_init(struct cifs_tcon *tcon, struct TCP_Server_Info *server,
> > > >                  struct smb_rqst *rqst,
> > > >                  u64 persistent_fid, u64 volatile_fid, u32 opcode,
> > > > -               bool is_fsctl, char *in_data, u32 indatalen,
> > > > +               char *in_data, u32 indatalen,
> > > >                  __u32 max_response_size)
> > > >   {
> > > >          struct smb2_ioctl_req *req;
> > > > @@ -3131,10 +3131,8 @@ SMB2_ioctl_init(struct cifs_tcon *tcon, struct TCP_Server_Info *server,
> > > >          req->hdr.CreditCharge =
> > > >                  cpu_to_le16(DIV_ROUND_UP(max(indatalen, max_response_size),
> > > >                                           SMB2_MAX_BUFFER_SIZE));
> > > > -       if (is_fsctl)
> > > > -               req->Flags = cpu_to_le32(SMB2_0_IOCTL_IS_FSCTL);
> > > > -       else
> > > > -               req->Flags = 0;
> > > > +       /* always an FSCTL (for now) */
> > > > +       req->Flags = cpu_to_le32(SMB2_0_IOCTL_IS_FSCTL);
> > > >
> > > >          /* validate negotiate request must be signed - see MS-SMB2 3.2.5.5 */
> > > >          if (opcode == FSCTL_VALIDATE_NEGOTIATE_INFO)
> > > > @@ -3161,9 +3159,9 @@ SMB2_ioctl_free(struct smb_rqst *rqst)
> > > >    */
> > > >   int
> > > >   SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid,
> > > > -          u64 volatile_fid, u32 opcode, bool is_fsctl,
> > > > -          char *in_data, u32 indatalen, u32 max_out_data_len,
> > > > -          char **out_data, u32 *plen /* returned data len */)
> > > > +          u64 volatile_fid, u32 opcode, char *in_data, u32 indatalen,
> > > > +          u32 max_out_data_len, char **out_data,
> > > > +          u32 *plen /* returned data len */)
> > > >   {
> > > >          struct smb_rqst rqst;
> > > >          struct smb2_ioctl_rsp *rsp = NULL;
> > > > @@ -3205,7 +3203,7 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid,
> > > >
> > > >          rc = SMB2_ioctl_init(tcon, server,
> > > >                               &rqst, persistent_fid, volatile_fid, opcode,
> > > > -                            is_fsctl, in_data, indatalen, max_out_data_len);
> > > > +                            in_data, indatalen, max_out_data_len);
> > > >          if (rc)
> > > >                  goto ioctl_exit;
> > > >
> > > > @@ -3297,7 +3295,7 @@ SMB2_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
> > > >                          cpu_to_le16(COMPRESSION_FORMAT_DEFAULT);
> > > >
> > > >          rc = SMB2_ioctl(xid, tcon, persistent_fid, volatile_fid,
> > > > -                       FSCTL_SET_COMPRESSION, true /* is_fsctl */,
> > > > +                       FSCTL_SET_COMPRESSION,
> > > >                          (char *)&fsctl_input /* data input */,
> > > >                          2 /* in data len */, CIFSMaxBufSize /* max out data */,
> > > >                          &ret_data /* out data */, NULL);
> > > > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> > > > index 51c5bf4a338a..7001317de9dc 100644
> > > > --- a/fs/cifs/smb2proto.h
> > > > +++ b/fs/cifs/smb2proto.h
> > > > @@ -137,13 +137,13 @@ extern int SMB2_open_init(struct cifs_tcon *tcon,
> > > >   extern void SMB2_open_free(struct smb_rqst *rqst);
> > > >   extern int SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon,
> > > >                       u64 persistent_fid, u64 volatile_fid, u32 opcode,
> > > > -                    bool is_fsctl, char *in_data, u32 indatalen, u32 maxoutlen,
> > > > +                    char *in_data, u32 indatalen, u32 maxoutlen,
> > > >                       char **out_data, u32 *plen /* returned data len */);
> > > >   extern int SMB2_ioctl_init(struct cifs_tcon *tcon,
> > > >                             struct TCP_Server_Info *server,
> > > >                             struct smb_rqst *rqst,
> > > >                             u64 persistent_fid, u64 volatile_fid, u32 opcode,
> > > > -                          bool is_fsctl, char *in_data, u32 indatalen,
> > > > +                          char *in_data, u32 indatalen,
> > > >                             __u32 max_response_size);
> > > >   extern void SMB2_ioctl_free(struct smb_rqst *rqst);
> > > >   extern int SMB2_change_notify(const unsigned int xid, struct cifs_tcon *tcon,
> > > > --
> > > > 2.35.3
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve