| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20090220162711.4e056ab8@tupile.poochiereds.net>
Date: Fri, 20 Feb 2009 16:27:11 -0800
From: Jeff Layton <jlayton@...hat.com>
To: Horst Reiterer <horst.reiterer@...il.com>
Cc: smfrench@...il.com, linux-cifs-client@...ts.samba.org,
linux-kernel@...r.kernel.org
Subject: Re: [linux-cifs-client] [PATCH] fs/cifs: send SMB_COM_FLUSH in
cifs_fsync
On Fri, 20 Feb 2009 21:51:46 +0100 (CET)
Horst Reiterer <horst.reiterer@...il.com> wrote:
> Hi,
>
> In contrast to the now-obsolete smbfs, cifs does not send SMB_COM_FLUSH
> in response to an explicit fsync(2) to guarantee that all volatile data
> is written to stable storage on the server side, provided the server
> honors the request (which, to my knowledge, is true for Windows and
> Samba with 'strict sync' enabled).
> This patch modifies the cifs_fsync implementation to restore the
> fsync-behavior of smbfs by triggering SMB_COM_FLUSH after sending
> outstanding data on the client side to the server.
>
> I inquired about this issue in the linux-cifs-client@...ts.samba.org
> mailing list:
>
> On Tue, Feb 10, 2009 at 12:35 AM, Jeff Layton <jlayton@...hat.com> wrote:
> > Horst Reiterer wrote:
> > > Why was the explicit SMB_COM_FLUSH dropped in the new implementation?
> >
> > It's not that it was "removed" per-se, just never implemented. Patches
> > to add that capability would certainly be welcome.
>
> I tested the patch with 2.6.28.6 and 2.6.18 (backported) on x86_64.
> Please review and apply.
>
> Horst.
>
>
> Signed-off-by: Horst Reiterer <horst.reiterer@...il.com>
>
Thanks for doing this. Comments below...
> diff -uprN linux-2.6.28.6/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
> --- linux-2.6.28.6/fs/cifs/cifs_debug.c 2009-02-17 18:29:27.000000000 +0100
> +++ b/fs/cifs/cifs_debug.c 2009-02-20 00:42:18.000000000 +0100
> @@ -340,6 +340,8 @@ static int cifs_stats_proc_show(struct s
> seq_printf(m, "\nWrites: %d Bytes: %lld",
> atomic_read(&tcon->num_writes),
> (long long)(tcon->bytes_written));
> + seq_printf(m, "\nFlushes: %d",
> + atomic_read(&tcon->num_flushes));
> seq_printf(m, "\nLocks: %d HardLinks: %d "
> "Symlinks: %d",
> atomic_read(&tcon->num_locks),
> diff -uprN linux-2.6.28.6/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> --- linux-2.6.28.6/fs/cifs/cifsglob.h 2009-02-17 18:29:27.000000000 +0100
> +++ b/fs/cifs/cifsglob.h 2009-02-20 00:42:18.000000000 +0100
> @@ -246,6 +246,7 @@ struct cifsTconInfo {
> atomic_t num_smbs_sent;
> atomic_t num_writes;
> atomic_t num_reads;
> + atomic_t num_flushes;
> atomic_t num_oplock_brks;
> atomic_t num_opens;
> atomic_t num_closes;
> diff -uprN linux-2.6.28.6/fs/cifs/cifspdu.h b/fs/cifs/cifspdu.h
> --- linux-2.6.28.6/fs/cifs/cifspdu.h 2009-02-17 18:29:27.000000000 +0100
> +++ b/fs/cifs/cifspdu.h 2009-02-20 00:42:18.000000000 +0100
> @@ -43,6 +43,7 @@
> #define SMB_COM_CREATE_DIRECTORY 0x00 /* trivial response */
> #define SMB_COM_DELETE_DIRECTORY 0x01 /* trivial response */
> #define SMB_COM_CLOSE 0x04 /* triv req/rsp, timestamp ignored */
> +#define SMB_COM_FLUSH 0x05 /* triv req/rsp */
> #define SMB_COM_DELETE 0x06 /* trivial response */
> #define SMB_COM_RENAME 0x07 /* trivial response */
> #define SMB_COM_QUERY_INFORMATION 0x08 /* aka getattr */
> @@ -790,6 +791,12 @@ typedef struct smb_com_close_rsp {
> __u16 ByteCount; /* bct = 0 */
> } __attribute__((packed)) CLOSE_RSP;
>
> +typedef struct smb_com_flush_req {
> + struct smb_hdr hdr; /* wct = 1 */
> + __u16 FileID;
> + __u16 ByteCount; /* 0 */
> +} __attribute__((packed)) FLUSH_REQ;
> +
> typedef struct smb_com_findclose_req {
> struct smb_hdr hdr; /* wct = 1 */
> __u16 FileID;
> diff -uprN linux-2.6.28.6/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> --- linux-2.6.28.6/fs/cifs/cifsproto.h 2009-02-17 18:29:27.000000000 +0100
> +++ b/fs/cifs/cifsproto.h 2009-02-20 00:42:18.000000000 +0100
> @@ -277,6 +277,9 @@ extern int CIFSPOSIXCreate(const int xid
> extern int CIFSSMBClose(const int xid, struct cifsTconInfo *tcon,
> const int smb_file_id);
>
> +extern int CIFSSMBFlush(const int xid, struct cifsTconInfo *tcon,
> + const int smb_file_id);
> +
> extern int CIFSSMBRead(const int xid, struct cifsTconInfo *tcon,
> const int netfid, unsigned int count,
> const __u64 lseek, unsigned int *nbytes, char **buf,
> diff -uprN linux-2.6.28.6/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> --- linux-2.6.28.6/fs/cifs/cifssmb.c 2009-02-17 18:29:27.000000000 +0100
> +++ b/fs/cifs/cifssmb.c 2009-02-20 00:42:18.000000000 +0100
> @@ -1928,6 +1928,27 @@ CIFSSMBClose(const int xid, struct cifsT
> }
>
> int
> +CIFSSMBFlush(const int xid, struct cifsTconInfo *tcon, int smb_file_id)
> +{
> + int rc = 0;
> + FLUSH_REQ *pSMB = NULL;
> + cFYI(1, ("In CIFSSMBFlush"));
> +
> + rc = small_smb_init(SMB_COM_FLUSH, 1, tcon, (void **) &pSMB);
> + if (rc)
> + return rc;
> +
> + pSMB->FileID = (__u16) smb_file_id;
> + pSMB->ByteCount = 0;
> + rc = SendReceiveNoRsp(xid, tcon->ses, (struct smb_hdr *) pSMB, 0);
^^^
Seems to me like we should wait for the response here. If there's an error from the flush, shouldn't we report that to the caller of fsync()?
> + cifs_stats_inc(&tcon->num_flushes);
> + if (rc)
> + cERROR(1, ("Send error in Flush = %d", rc));
> +
> + return rc;
> +}
> +
> +int
> CIFSSMBRename(const int xid, struct cifsTconInfo *tcon,
> const char *fromName, const char *toName,
> const struct nls_table *nls_codepage, int remap)
> diff -uprN linux-2.6.28.6/fs/cifs/file.c b/fs/cifs/file.c
> --- linux-2.6.28.6/fs/cifs/file.c 2009-02-17 18:29:27.000000000 +0100
> +++ b/fs/cifs/file.c 2009-02-20 00:42:18.000000000 +0100
> @@ -1522,18 +1522,31 @@ int cifs_fsync(struct file *file, struct
> {
> int xid;
> int rc = 0;
> + struct cifs_sb_info *cifs_sb;
> + struct cifsTconInfo *pTcon;
> + struct cifsFileInfo *pSMBFile =
> + (struct cifsFileInfo *)file->private_data;
> struct inode *inode = file->f_path.dentry->d_inode;
>
> xid = GetXid();
>
> + cifs_sb = CIFS_SB(inode->i_sb);
> + pTcon = cifs_sb->tcon;
> +
> cFYI(1, ("Sync file - name: %s datasync: 0x%x",
> dentry->d_name.name, datasync));
>
> - rc = filemap_write_and_wait(inode->i_mapping);
> - if (rc == 0) {
> - rc = CIFS_I(inode)->write_behind_rc;
> - CIFS_I(inode)->write_behind_rc = 0;
> - }
> + if (pSMBFile) {
> + rc = filemap_write_and_wait(inode->i_mapping);
> + if (rc == 0) {
> + rc = CIFS_I(inode)->write_behind_rc;
> + CIFS_I(inode)->write_behind_rc = 0;
> + if (pTcon)
> + rc = CIFSSMBFlush(xid, pTcon, pSMBFile->netfid);
> + }
> + } else
> + rc = -EBADF;
> +
> FreeXid(xid);
> return rc;
> }
--
Jeff Layton <jlayton@...hat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists