On Friday 27 September 2024 14:54:31 Enzo Matsumiya wrote:
> Hi Pali,
>
> On 09/15, Pali Rohár wrote:
> > Linux cifs client can already detect SFU symlinks and reads it content
> > (target location). But currently is not able to create new symlink. So
> > implement this missing support.
> >
> > When 'sfu' mount option is specified and 'mfsymlinks' is not specified then
> > create new symlinks in SFU-style. This will provide full SFU compatibility
> > of symlinks when mounting cifs share with 'sfu' option. 'mfsymlinks' option
> > override SFU for better Apple compatibility as explained in fs_context.c
> > file in smb3_update_mnt_flags() function.
> >
> > Extend __cifs_sfu_make_node() function, which now can handle also S_IFLNK
> > type and refactor structures passed to sync_write() in this function, by
> > splitting SFU type and SFU data from original combined struct win_dev as
> > combined fixed-length struct cannot be used for variable-length symlinks.
> >
> > Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> > ---
> > fs/smb/client/cifspdu.h | 6 ---
> > fs/smb/client/cifsproto.h | 4 ++
> > fs/smb/client/fs_context.c | 13 ++++---
> > fs/smb/client/link.c | 3 ++
> > fs/smb/client/smb2ops.c | 80 +++++++++++++++++++++++++++++---------
> > 5 files changed, 77 insertions(+), 29 deletions(-)
> >
> > diff --git a/fs/smb/client/cifspdu.h b/fs/smb/client/cifspdu.h
> > index a2072ab9e586..c3b6263060b0 100644
> > --- a/fs/smb/client/cifspdu.h
> > +++ b/fs/smb/client/cifspdu.h
> > @@ -2573,12 +2573,6 @@ typedef struct {
> > } __attribute__((packed)) FIND_FILE_STANDARD_INFO; /* level 0x1 FF resp data */
> >
> >
> > -struct win_dev {
> > - unsigned char type[8]; /* IntxCHR or IntxBLK or LnxFIFO or LnxSOCK */
> > - __le64 major;
> > - __le64 minor;
> > -} __attribute__((packed));
> > -
> > struct fea {
> > unsigned char EA_flags;
> > __u8 name_len;
> > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> > index 497bf3c447bc..791bddac0396 100644
> > --- a/fs/smb/client/cifsproto.h
> > +++ b/fs/smb/client/cifsproto.h
> > @@ -676,6 +676,10 @@ char *extract_sharename(const char *unc);
> > int parse_reparse_point(struct reparse_data_buffer *buf,
> > u32 plen, struct cifs_sb_info *cifs_sb,
> > bool unicode, struct cifs_open_info_data *data);
> > +int __cifs_sfu_make_node(unsigned int xid, struct inode *inode,
> > + struct dentry *dentry, struct cifs_tcon *tcon,
> > + const char *full_path, umode_t mode, dev_t dev,
> > + const char *symname);
> > int cifs_sfu_make_node(unsigned int xid, struct inode *inode,
> > struct dentry *dentry, struct cifs_tcon *tcon,
> > const char *full_path, umode_t mode, dev_t dev);
> > diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> > index bc926ab2555b..2f0c3894b0f7 100644
> > --- a/fs/smb/client/fs_context.c
> > +++ b/fs/smb/client/fs_context.c
> > @@ -1896,14 +1896,17 @@ void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb)
> > if (ctx->mfsymlinks) {
> > if (ctx->sfu_emul) {
> > /*
> > - * Our SFU ("Services for Unix" emulation does not allow
> > - * creating symlinks but does allow reading existing SFU
> > - * symlinks (it does allow both creating and reading SFU
> > - * style mknod and FIFOs though). When "mfsymlinks" and
> > + * Our SFU ("Services for Unix") emulation allows now
> > + * creating new and reading existing SFU symlinks.
> > + * Older Linux kernel versions were not able to neither
> > + * read existing nor create new SFU symlinks. But
> > + * creating and reading SFU style mknod and FIFOs was
> > + * supported for long time. When "mfsymlinks" and
> > * "sfu" are both enabled at the same time, it allows
> > * reading both types of symlinks, but will only create
> > * them with mfsymlinks format. This allows better
> > - * Apple compatibility (probably better for Samba too)
> > + * Apple compatibility, compatibility with older Linux
> > + * kernel clients (probably better for Samba too)
> > * while still recognizing old Windows style symlinks.
> > */
> > cifs_dbg(VFS, "mount options mfsymlinks and sfu both enabled\n");
> > diff --git a/fs/smb/client/link.c b/fs/smb/client/link.c
> > index 80099bbb333b..47ddeb7fa111 100644
> > --- a/fs/smb/client/link.c
> > +++ b/fs/smb/client/link.c
> > @@ -606,6 +606,9 @@ cifs_symlink(struct mnt_idmap *idmap, struct inode *inode,
> > /* BB what if DFS and this volume is on different share? BB */
> > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MF_SYMLINKS) {
> > rc = create_mf_symlink(xid, pTcon, cifs_sb, full_path, symname);
> > + } else if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL) {
> > + rc = __cifs_sfu_make_node(xid, inode, direntry, pTcon,
> > + full_path, S_IFLNK, 0, symname);
> > #ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
> > } else if (pTcon->unix_ext) {
> > rc = CIFSUnixCreateSymLink(xid, pTcon, full_path, symname,
> > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> > index 9c2d065d3cc4..2c251e9a3a30 100644
> > --- a/fs/smb/client/smb2ops.c
> > +++ b/fs/smb/client/smb2ops.c
> > @@ -5055,9 +5055,10 @@ static int smb2_next_header(struct TCP_Server_Info *server, char *buf,
> > return 0;
> > }
> >
> > -static int __cifs_sfu_make_node(unsigned int xid, struct inode *inode,
> > +int __cifs_sfu_make_node(unsigned int xid, struct inode *inode,
> > struct dentry *dentry, struct cifs_tcon *tcon,
> > - const char *full_path, umode_t mode, dev_t dev)
> > + const char *full_path, umode_t mode, dev_t dev,
> > + const char *symname)
> > {
> > struct TCP_Server_Info *server = tcon->ses->server;
> > struct cifs_open_parms oparms;
> > @@ -5065,30 +5066,64 @@ static int __cifs_sfu_make_node(unsigned int xid, struct inode *inode,
> > struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> > struct cifs_fid fid;
> > unsigned int bytes_written;
> > - struct win_dev pdev = {};
> > - struct kvec iov[2];
> > + u8 type[8];
> > + int type_len = 0;
> > + struct {
> > + __le64 major;
> > + __le64 minor;
> > + } __packed pdev = {};
> > + __le16 *symname_utf16 = NULL;
> > + u8 *data = NULL;
> > + int data_len = 0;
> > + struct kvec iov[3];
> > __u32 oplock = server->oplocks ? REQ_OPLOCK : 0;
> > int rc;
> >
> > switch (mode & S_IFMT) {
> > case S_IFCHR:
> > - memcpy(pdev.type, "IntxCHR\0", 8);
> > + type_len = 8;
> > + memcpy(type, "IntxCHR\0", type_len);
> > pdev.major = cpu_to_le64(MAJOR(dev));
> > pdev.minor = cpu_to_le64(MINOR(dev));
> > + data = (u8 *)&pdev;
> > + data_len = sizeof(pdev);
> > break;
> > case S_IFBLK:
> > - memcpy(pdev.type, "IntxBLK\0", 8);
> > + type_len = 8;
> > + memcpy(type, "IntxBLK\0", type_len);
> > pdev.major = cpu_to_le64(MAJOR(dev));
> > pdev.minor = cpu_to_le64(MINOR(dev));
> > + data = (u8 *)&pdev;
> > + data_len = sizeof(pdev);
> > + break;
> > + case S_IFLNK:
> > + type_len = 8;
> > + memcpy(type, "IntxLNK\1", type_len);
> > + symname_utf16 = cifs_strndup_to_utf16(symname, strlen(symname),
> > + &data_len, cifs_sb->local_nls,
> > + NO_MAP_UNI_RSVD);
> > + if (!symname_utf16) {
> > + rc = -ENOMEM;
> > + goto out;
> > + }
> > + data_len -= 2; /* symlink is without trailing wide-nul */
> > + data = (u8 *)symname_utf16;
>
> Can't S_IFLNK be handled somewhere else/other function? mknod doesn't
> support S_IFLNK, so this seems out of place.
>
> And even though it's unreachable (AFAICS), cifs_sfu_make_node() calls
> this with @symname == NULL (caught with gcc -fanalyzer).
>
>
> Cheers,
>
> Enzo
Hello! As SFU-style special files have same format, I just extended this
function which handles all SFU types, to handle also symlink.
I wanted to reuse existing function instead of copying and duplicating
code.
Parameter symname is used only for S_IFLNK, so this should be safe. And
as you pointed out mknod does not support S_IFLNK, so mknod code path
would not call __cifs_sfu_make_node() function with S_IFLNK argument.
So I think that there is not issue. But if you want to refactor this
code, do you have an idea what to do?
> > break;
> > case S_IFSOCK:
> > - strscpy(pdev.type, "LnxSOCK");
> > + type_len = 8;
> > + strscpy(type, "LnxSOCK");
> > + data = (u8 *)&pdev;
> > + data_len = sizeof(pdev);
> > break;
> > case S_IFIFO:
> > - strscpy(pdev.type, "LnxFIFO");
> > + type_len = 8;
> > + strscpy(type, "LnxFIFO");
> > + data = (u8 *)&pdev;
> > + data_len = sizeof(pdev);
> > break;
> > default:
> > - return -EPERM;
> > + rc = -EPERM;
> > + goto out;
> > }
> >
> > oparms = CIFS_OPARMS(cifs_sb, tcon, full_path, GENERIC_WRITE,
> > @@ -5098,17 +5133,26 @@ static int __cifs_sfu_make_node(unsigned int xid, struct inode *inode,
> >
> > rc = server->ops->open(xid, &oparms, &oplock, NULL);
> > if (rc)
> > - return rc;
> > + goto out;
> >
> > - io_parms.pid = current->tgid;
> > - io_parms.tcon = tcon;
> > - io_parms.length = sizeof(pdev);
> > - iov[1].iov_base = &pdev;
> > - iov[1].iov_len = sizeof(pdev);
> > + if (type_len + data_len > 0) {
> > + io_parms.pid = current->tgid;
> > + io_parms.tcon = tcon;
> > + io_parms.length = type_len + data_len;
> > + iov[1].iov_base = type;
> > + iov[1].iov_len = type_len;
> > + iov[2].iov_base = data;
> > + iov[2].iov_len = data_len;
> > +
> > + rc = server->ops->sync_write(xid, &fid, &io_parms,
> > + &bytes_written,
> > + iov, ARRAY_SIZE(iov)-1);
> > + }
> >
> > - rc = server->ops->sync_write(xid, &fid, &io_parms,
> > - &bytes_written, iov, 1);
> > server->ops->close(xid, tcon, &fid);
> > +
> > +out:
> > + kfree(symname_utf16);
> > return rc;
> > }
> >
> > @@ -5120,7 +5164,7 @@ int cifs_sfu_make_node(unsigned int xid, struct inode *inode,
> > int rc;
> >
> > rc = __cifs_sfu_make_node(xid, inode, dentry, tcon,
> > - full_path, mode, dev);
> > + full_path, mode, dev, NULL);
> > if (rc)
> > return rc;
