from:"Shirish Pargaonkar"

Re: [PATCH] cifs: cifsacl: Use a temporary ops variable to reduce code length

2017-05-09 Thread Shirish Pargaonkar

Looks correct.

Acked-by: Shirish Pargaonkar <shirishpargaon...@gmail.com>

On Sun, May 7, 2017 at 3:31 AM, Joe Perches via samba-technical
<samba-techni...@lists.samba.org> wrote:
> Create an ops variable to store tcon->ses->server->ops and cache
> indirections and reduce code size a trivial bit.
>
> $ size fs/cifs/cifsacl.o*
>textdata bss dec hex filename
>5338 136   85482156a fs/cifs/cifsacl.o.new
>5371 136   85515158b fs/cifs/cifsacl.o.old
>
> Signed-off-by: Joe Perches <j...@perches.com>
> ---
>  fs/cifs/cifsacl.c | 30 ++
>  1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> index 15bac390dff9..b98436f5c7c7 100644
> --- a/fs/cifs/cifsacl.c
> +++ b/fs/cifs/cifsacl.c
> @@ -1135,20 +1135,19 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, 
> struct cifs_fattr *fattr,
> u32 acllen = 0;
> int rc = 0;
> struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
> -   struct cifs_tcon *tcon;
> +   struct smb_version_operations *ops;
>
> cifs_dbg(NOISY, "converting ACL to mode for %s\n", path);
>
> if (IS_ERR(tlink))
> return PTR_ERR(tlink);
> -   tcon = tlink_tcon(tlink);
>
> -   if (pfid && (tcon->ses->server->ops->get_acl_by_fid))
> -   pntsd = tcon->ses->server->ops->get_acl_by_fid(cifs_sb, pfid,
> - );
> -   else if (tcon->ses->server->ops->get_acl)
> -   pntsd = tcon->ses->server->ops->get_acl(cifs_sb, inode, path,
> -   );
> +   ops = tlink_tcon(tlink)->ses->server->ops;
> +
> +   if (pfid && (ops->get_acl_by_fid))
> +   pntsd = ops->get_acl_by_fid(cifs_sb, pfid, );
> +   else if (ops->get_acl)
> +   pntsd = ops->get_acl(cifs_sb, inode, path, );
> else {
> cifs_put_tlink(tlink);
> return -EOPNOTSUPP;
> @@ -1181,23 +1180,23 @@ id_mode_to_cifs_acl(struct inode *inode, const char 
> *path, __u64 nmode,
> struct cifs_ntsd *pnntsd = NULL; /* modified acl to be sent to server 
> */
> struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
> -   struct cifs_tcon *tcon;
> +   struct smb_version_operations *ops;
>
> if (IS_ERR(tlink))
> return PTR_ERR(tlink);
> -   tcon = tlink_tcon(tlink);
> +
> +   ops = tlink_tcon(tlink)->ses->server->ops;
>
> cifs_dbg(NOISY, "set ACL from mode for %s\n", path);
>
> /* Get the security descriptor */
>
> -   if (tcon->ses->server->ops->get_acl == NULL) {
> +   if (ops->get_acl == NULL) {
> cifs_put_tlink(tlink);
> return -EOPNOTSUPP;
> }
>
> -   pntsd = tcon->ses->server->ops->get_acl(cifs_sb, inode, path,
> -   );
> +   pntsd = ops->get_acl(cifs_sb, inode, path, );
> if (IS_ERR(pntsd)) {
> rc = PTR_ERR(pntsd);
> cifs_dbg(VFS, "%s: error %d getting sec desc\n", __func__, 
> rc);
> @@ -1224,13 +1223,12 @@ id_mode_to_cifs_acl(struct inode *inode, const char 
> *path, __u64 nmode,
>
> cifs_dbg(NOISY, "build_sec_desc rc: %d\n", rc);
>
> -   if (tcon->ses->server->ops->set_acl == NULL)
> +   if (ops->set_acl == NULL)
> rc = -EOPNOTSUPP;
>
> if (!rc) {
> /* Set the security descriptor */
> -   rc = tcon->ses->server->ops->set_acl(pnntsd, secdesclen, 
> inode,
> -path, aclflag);
> +   rc = ops->set_acl(pnntsd, secdesclen, inode, path, aclflag);
> cifs_dbg(NOISY, "set_cifs_acl rc: %d\n", rc);
> }
> cifs_put_tlink(tlink);
> --
> 2.10.0.rc2.1.g053435c
>
>

Re: [PATCH] cifs: cifsacl: Use a temporary ops variable to reduce code length

2017-05-09 Thread Shirish Pargaonkar

Looks correct.

Acked-by: Shirish Pargaonkar 

On Sun, May 7, 2017 at 3:31 AM, Joe Perches via samba-technical
 wrote:
> Create an ops variable to store tcon->ses->server->ops and cache
> indirections and reduce code size a trivial bit.
>
> $ size fs/cifs/cifsacl.o*
>textdata bss dec hex filename
>5338 136   85482156a fs/cifs/cifsacl.o.new
>5371 136   85515158b fs/cifs/cifsacl.o.old
>
> Signed-off-by: Joe Perches 
> ---
>  fs/cifs/cifsacl.c | 30 ++
>  1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> index 15bac390dff9..b98436f5c7c7 100644
> --- a/fs/cifs/cifsacl.c
> +++ b/fs/cifs/cifsacl.c
> @@ -1135,20 +1135,19 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, 
> struct cifs_fattr *fattr,
> u32 acllen = 0;
> int rc = 0;
> struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
> -   struct cifs_tcon *tcon;
> +   struct smb_version_operations *ops;
>
> cifs_dbg(NOISY, "converting ACL to mode for %s\n", path);
>
> if (IS_ERR(tlink))
> return PTR_ERR(tlink);
> -   tcon = tlink_tcon(tlink);
>
> -   if (pfid && (tcon->ses->server->ops->get_acl_by_fid))
> -   pntsd = tcon->ses->server->ops->get_acl_by_fid(cifs_sb, pfid,
> - );
> -   else if (tcon->ses->server->ops->get_acl)
> -   pntsd = tcon->ses->server->ops->get_acl(cifs_sb, inode, path,
> -   );
> +   ops = tlink_tcon(tlink)->ses->server->ops;
> +
> +   if (pfid && (ops->get_acl_by_fid))
> +   pntsd = ops->get_acl_by_fid(cifs_sb, pfid, );
> +   else if (ops->get_acl)
> +   pntsd = ops->get_acl(cifs_sb, inode, path, );
> else {
> cifs_put_tlink(tlink);
> return -EOPNOTSUPP;
> @@ -1181,23 +1180,23 @@ id_mode_to_cifs_acl(struct inode *inode, const char 
> *path, __u64 nmode,
> struct cifs_ntsd *pnntsd = NULL; /* modified acl to be sent to server 
> */
> struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
> -   struct cifs_tcon *tcon;
> +   struct smb_version_operations *ops;
>
> if (IS_ERR(tlink))
> return PTR_ERR(tlink);
> -   tcon = tlink_tcon(tlink);
> +
> +   ops = tlink_tcon(tlink)->ses->server->ops;
>
> cifs_dbg(NOISY, "set ACL from mode for %s\n", path);
>
> /* Get the security descriptor */
>
> -   if (tcon->ses->server->ops->get_acl == NULL) {
> +   if (ops->get_acl == NULL) {
> cifs_put_tlink(tlink);
> return -EOPNOTSUPP;
> }
>
> -   pntsd = tcon->ses->server->ops->get_acl(cifs_sb, inode, path,
> -   );
> +   pntsd = ops->get_acl(cifs_sb, inode, path, );
> if (IS_ERR(pntsd)) {
> rc = PTR_ERR(pntsd);
> cifs_dbg(VFS, "%s: error %d getting sec desc\n", __func__, 
> rc);
> @@ -1224,13 +1223,12 @@ id_mode_to_cifs_acl(struct inode *inode, const char 
> *path, __u64 nmode,
>
> cifs_dbg(NOISY, "build_sec_desc rc: %d\n", rc);
>
> -   if (tcon->ses->server->ops->set_acl == NULL)
> +   if (ops->set_acl == NULL)
> rc = -EOPNOTSUPP;
>
> if (!rc) {
> /* Set the security descriptor */
> -   rc = tcon->ses->server->ops->set_acl(pnntsd, secdesclen, 
> inode,
> -path, aclflag);
> +   rc = ops->set_acl(pnntsd, secdesclen, inode, path, aclflag);
> cifs_dbg(NOISY, "set_cifs_acl rc: %d\n", rc);
> }
> cifs_put_tlink(tlink);
> --
> 2.10.0.rc2.1.g053435c
>
>

Re: [PATCH 2/6] cifs: merge the hash calculation helpers

2016-04-12 Thread Shirish Pargaonkar

Looks correct.
Tested with -o vers=1.0, -o vers=2.0, and -o vers=2.1.
Will test using mount option vers=3.0 as soon as I find a server.

Reviewed-by: Shirish Pargaonkar <shirishpargaon...@gmail.com>
Tested-by: Shirish Pargaonkar <shirishpargaon...@gmail.com>

On Sat, Apr 9, 2016 at 3:50 PM, Al Viro <v...@zeniv.linux.org.uk> wrote:
> three practically identical copies...
>
> Signed-off-by: Al Viro <v...@zeniv.linux.org.uk>
> ---
>  fs/cifs/cifsencrypt.c   |  97 ---
>  fs/cifs/cifsproto.h |   3 ++
>  fs/cifs/smb2transport.c | 107 
> +---
>  3 files changed, 67 insertions(+), 140 deletions(-)
>
> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> index 4897dac..6aeb8d4 100644
> --- a/fs/cifs/cifsencrypt.c
> +++ b/fs/cifs/cifsencrypt.c
> @@ -66,45 +66,15 @@ cifs_crypto_shash_md5_allocate(struct TCP_Server_Info 
> *server)
> return 0;
>  }
>
> -/*
> - * Calculate and return the CIFS signature based on the mac key and SMB PDU.
> - * The 16 byte signature must be allocated by the caller. Note we only use 
> the
> - * 1st eight bytes and that the smb header signature field on input contains
> - * the sequence number before this function is called. Also, this function
> - * should be called with the server->srv_mutex held.
> - */
> -static int cifs_calc_signature(struct smb_rqst *rqst,
> -   struct TCP_Server_Info *server, char *signature)
> +int __cifs_calc_signature(struct smb_rqst *rqst,
> +   struct TCP_Server_Info *server, char *signature,
> +   struct shash_desc *shash)
>  {
> int i;
> int rc;
> struct kvec *iov = rqst->rq_iov;
> int n_vec = rqst->rq_nvec;
>
> -   if (iov == NULL || signature == NULL || server == NULL)
> -   return -EINVAL;
> -
> -   if (!server->secmech.sdescmd5) {
> -   rc = cifs_crypto_shash_md5_allocate(server);
> -   if (rc) {
> -   cifs_dbg(VFS, "%s: Can't alloc md5 crypto\n", 
> __func__);
> -   return -1;
> -   }
> -   }
> -
> -   rc = crypto_shash_init(>secmech.sdescmd5->shash);
> -   if (rc) {
> -   cifs_dbg(VFS, "%s: Could not init md5\n", __func__);
> -   return rc;
> -   }
> -
> -   rc = crypto_shash_update(>secmech.sdescmd5->shash,
> -   server->session_key.response, server->session_key.len);
> -   if (rc) {
> -   cifs_dbg(VFS, "%s: Could not update with response\n", 
> __func__);
> -   return rc;
> -   }
> -
> for (i = 0; i < n_vec; i++) {
> if (iov[i].iov_len == 0)
> continue;
> @@ -117,12 +87,10 @@ static int cifs_calc_signature(struct smb_rqst *rqst,
> if (i == 0) {
> if (iov[0].iov_len <= 8) /* cmd field at offset 9 */
> break; /* nothing to sign or corrupt header */
> -   rc =
> -   crypto_shash_update(>secmech.sdescmd5->shash,
> +   rc = crypto_shash_update(shash,
> iov[i].iov_base + 4, iov[i].iov_len - 4);
> } else {
> -   rc =
> -   crypto_shash_update(>secmech.sdescmd5->shash,
> +   rc = crypto_shash_update(shash,
> iov[i].iov_base, iov[i].iov_len);
> }
> if (rc) {
> @@ -134,21 +102,64 @@ static int cifs_calc_signature(struct smb_rqst *rqst,
>
> /* now hash over the rq_pages array */
> for (i = 0; i < rqst->rq_npages; i++) {
> -   struct kvec p_iov;
> +   void *kaddr = kmap(rqst->rq_pages[i]);
> +   size_t len = rqst->rq_pagesz;
> +
> +   if (i == rqst->rq_npages - 1)
> +   len = rqst->rq_tailsz;
> +
> +   crypto_shash_update(shash, kaddr, len);
>
> -   cifs_rqst_page_to_kvec(rqst, i, _iov);
> -   crypto_shash_update(>secmech.sdescmd5->shash,
> -   p_iov.iov_base, p_iov.iov_len);
> kunmap(rqst->rq_pages[i]);
> }
>
> -   rc = crypto_shash_final(>secmech.sdescmd5->shash, signature);
> +   rc = crypto_shash_final(shash, signature);
> if (rc)
> -   cifs_dbg(VFS, "%s: Could not generate md5 hash\n"

Re: [PATCH 2/6] cifs: merge the hash calculation helpers

2016-04-12 Thread Shirish Pargaonkar

Looks correct.
Tested with -o vers=1.0, -o vers=2.0, and -o vers=2.1.
Will test using mount option vers=3.0 as soon as I find a server.

Reviewed-by: Shirish Pargaonkar 
Tested-by: Shirish Pargaonkar 

On Sat, Apr 9, 2016 at 3:50 PM, Al Viro  wrote:
> three practically identical copies...
>
> Signed-off-by: Al Viro 
> ---
>  fs/cifs/cifsencrypt.c   |  97 ---
>  fs/cifs/cifsproto.h |   3 ++
>  fs/cifs/smb2transport.c | 107 
> +---
>  3 files changed, 67 insertions(+), 140 deletions(-)
>
> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> index 4897dac..6aeb8d4 100644
> --- a/fs/cifs/cifsencrypt.c
> +++ b/fs/cifs/cifsencrypt.c
> @@ -66,45 +66,15 @@ cifs_crypto_shash_md5_allocate(struct TCP_Server_Info 
> *server)
> return 0;
>  }
>
> -/*
> - * Calculate and return the CIFS signature based on the mac key and SMB PDU.
> - * The 16 byte signature must be allocated by the caller. Note we only use 
> the
> - * 1st eight bytes and that the smb header signature field on input contains
> - * the sequence number before this function is called. Also, this function
> - * should be called with the server->srv_mutex held.
> - */
> -static int cifs_calc_signature(struct smb_rqst *rqst,
> -   struct TCP_Server_Info *server, char *signature)
> +int __cifs_calc_signature(struct smb_rqst *rqst,
> +   struct TCP_Server_Info *server, char *signature,
> +   struct shash_desc *shash)
>  {
> int i;
> int rc;
> struct kvec *iov = rqst->rq_iov;
> int n_vec = rqst->rq_nvec;
>
> -   if (iov == NULL || signature == NULL || server == NULL)
> -   return -EINVAL;
> -
> -   if (!server->secmech.sdescmd5) {
> -   rc = cifs_crypto_shash_md5_allocate(server);
> -   if (rc) {
> -   cifs_dbg(VFS, "%s: Can't alloc md5 crypto\n", 
> __func__);
> -   return -1;
> -   }
> -   }
> -
> -   rc = crypto_shash_init(>secmech.sdescmd5->shash);
> -   if (rc) {
> -   cifs_dbg(VFS, "%s: Could not init md5\n", __func__);
> -   return rc;
> -   }
> -
> -   rc = crypto_shash_update(>secmech.sdescmd5->shash,
> -   server->session_key.response, server->session_key.len);
> -   if (rc) {
> -   cifs_dbg(VFS, "%s: Could not update with response\n", 
> __func__);
> -   return rc;
> -   }
> -
> for (i = 0; i < n_vec; i++) {
> if (iov[i].iov_len == 0)
> continue;
> @@ -117,12 +87,10 @@ static int cifs_calc_signature(struct smb_rqst *rqst,
> if (i == 0) {
> if (iov[0].iov_len <= 8) /* cmd field at offset 9 */
> break; /* nothing to sign or corrupt header */
> -   rc =
> -   crypto_shash_update(>secmech.sdescmd5->shash,
> +   rc = crypto_shash_update(shash,
> iov[i].iov_base + 4, iov[i].iov_len - 4);
> } else {
> -   rc =
> -   crypto_shash_update(>secmech.sdescmd5->shash,
> +   rc = crypto_shash_update(shash,
> iov[i].iov_base, iov[i].iov_len);
> }
> if (rc) {
> @@ -134,21 +102,64 @@ static int cifs_calc_signature(struct smb_rqst *rqst,
>
> /* now hash over the rq_pages array */
> for (i = 0; i < rqst->rq_npages; i++) {
> -   struct kvec p_iov;
> +   void *kaddr = kmap(rqst->rq_pages[i]);
> +   size_t len = rqst->rq_pagesz;
> +
> +   if (i == rqst->rq_npages - 1)
> +   len = rqst->rq_tailsz;
> +
> +   crypto_shash_update(shash, kaddr, len);
>
> -   cifs_rqst_page_to_kvec(rqst, i, _iov);
> -   crypto_shash_update(>secmech.sdescmd5->shash,
> -   p_iov.iov_base, p_iov.iov_len);
> kunmap(rqst->rq_pages[i]);
> }
>
> -   rc = crypto_shash_final(>secmech.sdescmd5->shash, signature);
> +   rc = crypto_shash_final(shash, signature);
> if (rc)
> -   cifs_dbg(VFS, "%s: Could not generate md5 hash\n", __func__);
> +   cifs_dbg(VFS, "%s: Could not generate hash\n", __func__);
>
> return rc;

Re: [RFC][PATCHSET] reduce messing with iovecs in cifs

2016-04-10 Thread Shirish Pargaonkar

Looking at the series. I briefly looked at 2/6 and looks correct but
would add tested-by once I test that patch
against a SMB server.

On Sat, Apr 9, 2016 at 3:43 PM, Al Viro  wrote:
> Now that sendmsg/recvmsg do not mangle iovecs and are capable of
> handling bvec-based ->msg_iter, we can seriously reduce the amount of PITA
> in cifs.  The series below is completely untested, and I would appreciate
> comments/review/testing/etc.
>
> I'll post the individual patches in followups; for those who prefer to use
> git it can be found in
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git sendmsg.cifs
>
> 1/6: [net] drop 'size' argument of sock_recvmsg()
> should go via net-next; does what it says.
>
> 2/6: cifs: merge the hash calculation helpers
> takes the common parts of {cifs,smb2,smb3}_calc_signature() into a
> common helper.
>
> 3/6: cifs: quit playing games with draining iovecs
> Switch smb_send_kvec() to passing msghdr (and thus iov_iter) and
> make it use sock_sendmsg() - that allows to avoid draining iovecs, since
> ->msg_iter will be advanced properly and all we need is to keep it around
> between the calls of sock_sendmsg(), rather than reinitializing it on each
> loop iteration.  The same thing allows to get rid of messing with kmap()
> when sending the stuff in ->rq_pages[] - ITER_BVEC will do the right thing.
>
> 4/6: cifs: no need to wank with copying and advancing iovec on recvmsg side 
> either
> Similar to the previous - use sock_recvmsg() in 
> cifs_readv_from_socket()
> and there's no need to modify iovecs, or allocate a copy especially for
> such modifications, etc.
>
> 5/6: cifs_readv_receive: use cifs_read_from_socket()
> building a 1-element iovec array for cifs_readv_from_socket() is
> an overkill - simple cifs_read_from_socket() will do just fine.
>
> 6/6: cifs: don't bother with kmap on read_pages side
> Similar to the other half of 3/6: we can use ITER_BVEC for
> read-into-page case.  Just make cifs_readv_from_socket() take msghdr from
> caller and use a helper that would feed it a bvec-backed ->msg_iter.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCHSET] reduce messing with iovecs in cifs

2016-04-10 Thread Shirish Pargaonkar

Looking at the series. I briefly looked at 2/6 and looks correct but
would add tested-by once I test that patch
against a SMB server.

On Sat, Apr 9, 2016 at 3:43 PM, Al Viro  wrote:
> Now that sendmsg/recvmsg do not mangle iovecs and are capable of
> handling bvec-based ->msg_iter, we can seriously reduce the amount of PITA
> in cifs.  The series below is completely untested, and I would appreciate
> comments/review/testing/etc.
>
> I'll post the individual patches in followups; for those who prefer to use
> git it can be found in
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git sendmsg.cifs
>
> 1/6: [net] drop 'size' argument of sock_recvmsg()
> should go via net-next; does what it says.
>
> 2/6: cifs: merge the hash calculation helpers
> takes the common parts of {cifs,smb2,smb3}_calc_signature() into a
> common helper.
>
> 3/6: cifs: quit playing games with draining iovecs
> Switch smb_send_kvec() to passing msghdr (and thus iov_iter) and
> make it use sock_sendmsg() - that allows to avoid draining iovecs, since
> ->msg_iter will be advanced properly and all we need is to keep it around
> between the calls of sock_sendmsg(), rather than reinitializing it on each
> loop iteration.  The same thing allows to get rid of messing with kmap()
> when sending the stuff in ->rq_pages[] - ITER_BVEC will do the right thing.
>
> 4/6: cifs: no need to wank with copying and advancing iovec on recvmsg side 
> either
> Similar to the previous - use sock_recvmsg() in 
> cifs_readv_from_socket()
> and there's no need to modify iovecs, or allocate a copy especially for
> such modifications, etc.
>
> 5/6: cifs_readv_receive: use cifs_read_from_socket()
> building a 1-element iovec array for cifs_readv_from_socket() is
> an overkill - simple cifs_read_from_socket() will do just fine.
>
> 6/6: cifs: don't bother with kmap on read_pages side
> Similar to the other half of 3/6: we can use ITER_BVEC for
> read-into-page case.  Just make cifs_readv_from_socket() take msghdr from
> caller and use a helper that would feed it a bvec-backed ->msg_iter.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] [SMB3] Fix coverity warning

2015-03-28 Thread Shirish Pargaonkar

Looks correct.

Acked-by: Shirish Pargaonkar 

On Fri, Mar 27, 2015 at 12:28 AM, Steve French  wrote:
> Coverity reports a warning for referencing the beginning of the
> SMB2/SMB3 frame using the ProtocolId field as an array. Although
> it works the same either way, this patch should quiet the warning
> and might be a little clearer.
>
> Reported by Coverity (CID 741269)
>
> Signed-off-by: Steve French 
> ---
>  fs/cifs/smb2misc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> index 689f035..22dfdf1 100644
> --- a/fs/cifs/smb2misc.c
> +++ b/fs/cifs/smb2misc.c
> @@ -322,7 +322,7 @@ smb2_get_data_area_len(int *off, int *len, struct 
> smb2_hdr *hdr)
>
> /* return pointer to beginning of data area, ie offset from SMB start 
> */
> if ((*off != 0) && (*len != 0))
> -   return hdr->ProtocolId + *off;
> +   return (char *)(>ProtocolId[0]) + *off;
> else
> return NULL;
>  }
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning

2015-03-28 Thread Shirish Pargaonkar

Looks correct.

Acked-by: Shirish Pargaonkar 

On Fri, Mar 27, 2015 at 12:28 AM, Steve French  wrote:
> null tcon is not likely in these paths in current
> code, but obviously it does clarify the code to
> check for null (if at all) before derefrencing
> rather than after.
>
> Reported by Coverity (CID 1042666)
>
> Signed-off-by: Steve French 
> ---
>  fs/cifs/smb2pdu.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 1b906de..78b329f 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1218,7 +1218,7 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon 
> *tcon, u64 persistent_fid,
> struct smb2_ioctl_req *req;
> struct smb2_ioctl_rsp *rsp;
> struct TCP_Server_Info *server;
> -   struct cifs_ses *ses = tcon->ses;
> +   struct cifs_ses *ses;
> struct kvec iov[2];
> int resp_buftype;
> int num_iovecs;
> @@ -1233,6 +1233,11 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon 
> *tcon, u64 persistent_fid,
> if (plen)
> *plen = 0;
>
> +   if (tcon)
> +   ses = tcon->ses;
> +   else
> +   return -EIO;
> +
> if (ses && (ses->server))
> server = ses->server;
> else
> @@ -1296,14 +1301,12 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon 
> *tcon, u64 persistent_fid,
> rsp = (struct smb2_ioctl_rsp *)iov[0].iov_base;
>
> if ((rc != 0) && (rc != -EINVAL)) {
> -   if (tcon)
> -   cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE);
> +   cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE);
> goto ioctl_exit;
> } else if (rc == -EINVAL) {
> if ((opcode != FSCTL_SRV_COPYCHUNK_WRITE) &&
> (opcode != FSCTL_SRV_COPYCHUNK)) {
> -   if (tcon)
> -   cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE);
> +   cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE);
> goto ioctl_exit;
> }
> }
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon

2015-03-28 Thread Shirish Pargaonkar

Looks correct, although we could just goto a label at the end of this
function which
does the same.

Acked-by: Shirish Pargaonkar 


On Fri, Mar 27, 2015 at 12:28 AM, Steve French  wrote:
> Although unlikely to fail (and tree connect does not commonly send
> a password since SECMODE_USER is the default for most servers)
> do not ignore errors on SMBNTEncrypt in SMB Tree Connect.
>
> Reported by Coverity (CID 1226853)
>
> Signed-off-by: Steve French 
> ---
>  fs/cifs/connect.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 4cb8450..cdb1aaf 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -3696,6 +3696,12 @@ CIFSTCon(const unsigned int xid, struct cifs_ses *ses,
>  #endif /* CIFS_WEAK_PW_HASH */
> rc = SMBNTencrypt(tcon->password, ses->server->cryptkey,
> bcc_ptr, nls_codepage);
> +   if (rc) {
> +   cifs_dbg(FYI, "%s Can't generate NTLM rsp. Error: 
> %d\n",
> +__func__, rc);
> +   cifs_buf_release(smb_buffer);
> +   return rc;
> +   }
>
> bcc_ptr += CIFS_AUTH_RESP_SIZE;
> if (ses->capabilities & CAP_UNICODE) {
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype

2015-03-28 Thread Shirish Pargaonkar

Looks correct.

Acked-by: Shirish Pargaonkar 

On Fri, Mar 27, 2015 at 12:27 AM, Steve French  wrote:
> Pointed out by coverity analyzer.  resp_buftype is
> not initialized in one path which can rarely log
> a spurious warning (buf is null so there will
> not be a problem with freeing data, but if buf_type
> were randomly set to wrong value could log a warning)
>
> Reported by Coverity (CID 1269144)
>
> Signed-off-by: Steve French 
> ---
>  fs/cifs/smb2pdu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 3417340..1b906de 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -2114,7 +2114,7 @@ SMB2_query_directory(const unsigned int xid, struct 
> cifs_tcon *tcon,
> struct kvec iov[2];
> int rc = 0;
> int len;
> -   int resp_buftype;
> +   int resp_buftype = CIFS_NO_BUFFER;
> unsigned char *bufptr;
> struct TCP_Server_Info *server;
> struct cifs_ses *ses = tcon->ses;
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning

2015-03-28 Thread Shirish Pargaonkar

Looks correct.

Acked-by: Shirish Pargaonkar shirishpargaon...@gmail.com

On Fri, Mar 27, 2015 at 12:28 AM, Steve French smfre...@gmail.com wrote:
 null tcon is not likely in these paths in current
 code, but obviously it does clarify the code to
 check for null (if at all) before derefrencing
 rather than after.

 Reported by Coverity (CID 1042666)

 Signed-off-by: Steve French smfre...@gmail.com
 ---
  fs/cifs/smb2pdu.c | 13 -
  1 file changed, 8 insertions(+), 5 deletions(-)

 diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
 index 1b906de..78b329f 100644
 --- a/fs/cifs/smb2pdu.c
 +++ b/fs/cifs/smb2pdu.c
 @@ -1218,7 +1218,7 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon 
 *tcon, u64 persistent_fid,
 struct smb2_ioctl_req *req;
 struct smb2_ioctl_rsp *rsp;
 struct TCP_Server_Info *server;
 -   struct cifs_ses *ses = tcon-ses;
 +   struct cifs_ses *ses;
 struct kvec iov[2];
 int resp_buftype;
 int num_iovecs;
 @@ -1233,6 +1233,11 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon 
 *tcon, u64 persistent_fid,
 if (plen)
 *plen = 0;

 +   if (tcon)
 +   ses = tcon-ses;
 +   else
 +   return -EIO;
 +
 if (ses  (ses-server))
 server = ses-server;
 else
 @@ -1296,14 +1301,12 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon 
 *tcon, u64 persistent_fid,
 rsp = (struct smb2_ioctl_rsp *)iov[0].iov_base;

 if ((rc != 0)  (rc != -EINVAL)) {
 -   if (tcon)
 -   cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE);
 +   cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE);
 goto ioctl_exit;
 } else if (rc == -EINVAL) {
 if ((opcode != FSCTL_SRV_COPYCHUNK_WRITE) 
 (opcode != FSCTL_SRV_COPYCHUNK)) {
 -   if (tcon)
 -   cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE);
 +   cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE);
 goto ioctl_exit;
 }
 }
 --
 1.9.1

 --
 To unsubscribe from this list: send the line unsubscribe linux-cifs in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype

2015-03-28 Thread Shirish Pargaonkar

Looks correct.

Acked-by: Shirish Pargaonkar shirishpargaon...@gmail.com

On Fri, Mar 27, 2015 at 12:27 AM, Steve French smfre...@gmail.com wrote:
 Pointed out by coverity analyzer.  resp_buftype is
 not initialized in one path which can rarely log
 a spurious warning (buf is null so there will
 not be a problem with freeing data, but if buf_type
 were randomly set to wrong value could log a warning)

 Reported by Coverity (CID 1269144)

 Signed-off-by: Steve French smfre...@gmail.com
 ---
  fs/cifs/smb2pdu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
 index 3417340..1b906de 100644
 --- a/fs/cifs/smb2pdu.c
 +++ b/fs/cifs/smb2pdu.c
 @@ -2114,7 +2114,7 @@ SMB2_query_directory(const unsigned int xid, struct 
 cifs_tcon *tcon,
 struct kvec iov[2];
 int rc = 0;
 int len;
 -   int resp_buftype;
 +   int resp_buftype = CIFS_NO_BUFFER;
 unsigned char *bufptr;
 struct TCP_Server_Info *server;
 struct cifs_ses *ses = tcon-ses;
 --
 1.9.1

 --
 To unsubscribe from this list: send the line unsubscribe linux-cifs in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon

2015-03-28 Thread Shirish Pargaonkar

Looks correct, although we could just goto a label at the end of this
function which
does the same.

Acked-by: Shirish Pargaonkar shirishpargaon...@gmail.com


On Fri, Mar 27, 2015 at 12:28 AM, Steve French smfre...@gmail.com wrote:
 Although unlikely to fail (and tree connect does not commonly send
 a password since SECMODE_USER is the default for most servers)
 do not ignore errors on SMBNTEncrypt in SMB Tree Connect.

 Reported by Coverity (CID 1226853)

 Signed-off-by: Steve French smfre...@gmail.com
 ---
  fs/cifs/connect.c | 6 ++
  1 file changed, 6 insertions(+)

 diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
 index 4cb8450..cdb1aaf 100644
 --- a/fs/cifs/connect.c
 +++ b/fs/cifs/connect.c
 @@ -3696,6 +3696,12 @@ CIFSTCon(const unsigned int xid, struct cifs_ses *ses,
  #endif /* CIFS_WEAK_PW_HASH */
 rc = SMBNTencrypt(tcon-password, ses-server-cryptkey,
 bcc_ptr, nls_codepage);
 +   if (rc) {
 +   cifs_dbg(FYI, %s Can't generate NTLM rsp. Error: 
 %d\n,
 +__func__, rc);
 +   cifs_buf_release(smb_buffer);
 +   return rc;
 +   }

 bcc_ptr += CIFS_AUTH_RESP_SIZE;
 if (ses-capabilities  CAP_UNICODE) {
 --
 1.9.1

 --
 To unsubscribe from this list: send the line unsubscribe linux-cifs in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/4] [SMB3] Fix coverity warning

2015-03-28 Thread Shirish Pargaonkar

Looks correct.

Acked-by: Shirish Pargaonkar shirishpargaon...@gmail.com

On Fri, Mar 27, 2015 at 12:28 AM, Steve French smfre...@gmail.com wrote:
 Coverity reports a warning for referencing the beginning of the
 SMB2/SMB3 frame using the ProtocolId field as an array. Although
 it works the same either way, this patch should quiet the warning
 and might be a little clearer.

 Reported by Coverity (CID 741269)

 Signed-off-by: Steve French smfre...@gmail.com
 ---
  fs/cifs/smb2misc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
 index 689f035..22dfdf1 100644
 --- a/fs/cifs/smb2misc.c
 +++ b/fs/cifs/smb2misc.c
 @@ -322,7 +322,7 @@ smb2_get_data_area_len(int *off, int *len, struct 
 smb2_hdr *hdr)

 /* return pointer to beginning of data area, ie offset from SMB start 
 */
 if ((*off != 0)  (*len != 0))
 -   return hdr-ProtocolId + *off;
 +   return (char *)(hdr-ProtocolId[0]) + *off;
 else
 return NULL;
  }
 --
 1.9.1

 --
 To unsubscribe from this list: send the line unsubscribe linux-cifs in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Block: fix unbalanced bypass-disable in blk_register_queue

2014-09-09 Thread Shirish Pargaonkar

Tested-by: Shirish Pargaonkar 

On Tue, Sep 9, 2014 at 10:50 AM, Alan Stern  wrote:
> When a queue is registered, the block layer turns off the bypass
> setting (because bypass is enabled when the queue is created).  This
> doesn't work well for queues that are unregistered and then registered
> again; we get a WARNING because of the unbalanced calls to
> blk_queue_bypass_end().
>
> This patch fixes the problem by making blk_register_queue() call
> blk_queue_bypass_end() only the first time the queue is registered.
>
> Signed-off-by: Alan Stern 
> CC: Tejun Heo 
> CC: James Bottomley 
> CC: Jens Axboe 
>
> ---
>
> [as1765]
>
>
>  block/blk-sysfs.c |6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> Index: usb-3.17/block/blk-sysfs.c
> ===
> --- usb-3.17.orig/block/blk-sysfs.c
> +++ usb-3.17/block/blk-sysfs.c
> @@ -554,8 +554,10 @@ int blk_register_queue(struct gendisk *d
>  * Initialization must be complete by now.  Finish the initial
>  * bypass from queue allocation.
>  */
> -   queue_flag_set_unlocked(QUEUE_FLAG_INIT_DONE, q);
> -   blk_queue_bypass_end(q);
> +   if (!blk_queue_init_done(q)) {
> +   queue_flag_set_unlocked(QUEUE_FLAG_INIT_DONE, q);
> +   blk_queue_bypass_end(q);
> +   }
>
> ret = blk_trace_init_sysfs(dev);
> if (ret)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Block: fix unbalanced bypass-disable in blk_register_queue

2014-09-09 Thread Shirish Pargaonkar

Tested-by: Shirish Pargaonkar shirishpargaon...@gmail.com

On Tue, Sep 9, 2014 at 10:50 AM, Alan Stern st...@rowland.harvard.edu wrote:
 When a queue is registered, the block layer turns off the bypass
 setting (because bypass is enabled when the queue is created).  This
 doesn't work well for queues that are unregistered and then registered
 again; we get a WARNING because of the unbalanced calls to
 blk_queue_bypass_end().

 This patch fixes the problem by making blk_register_queue() call
 blk_queue_bypass_end() only the first time the queue is registered.

 Signed-off-by: Alan Stern st...@rowland.harvard.edu
 CC: Tejun Heo t...@kernel.org
 CC: James Bottomley james.bottom...@hansenpartnership.com
 CC: Jens Axboe ax...@kernel.dk

 ---

 [as1765]


  block/blk-sysfs.c |6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

 Index: usb-3.17/block/blk-sysfs.c
 ===
 --- usb-3.17.orig/block/blk-sysfs.c
 +++ usb-3.17/block/blk-sysfs.c
 @@ -554,8 +554,10 @@ int blk_register_queue(struct gendisk *d
  * Initialization must be complete by now.  Finish the initial
  * bypass from queue allocation.
  */
 -   queue_flag_set_unlocked(QUEUE_FLAG_INIT_DONE, q);
 -   blk_queue_bypass_end(q);
 +   if (!blk_queue_init_done(q)) {
 +   queue_flag_set_unlocked(QUEUE_FLAG_INIT_DONE, q);
 +   blk_queue_bypass_end(q);
 +   }

 ret = blk_trace_init_sysfs(dev);
 if (ret)

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: WARNING in block layer triggered in 3.17-rc3

2014-09-08 Thread Shirish Pargaonkar

see this issue/warning in 3.12 also.

On Mon, Sep 8, 2014 at 10:51 AM, James Bottomley
 wrote:
> On Mon, 2014-09-08 at 10:51 -0400, Alan Stern wrote:
>> On Sun, 7 Sep 2014, Shirish Pargaonkar wrote:
>>
>> > I think the problem is, when a gendisk is detached, its request queue
>> > is not put in bypass mode
>> > cause when it is re-attached, code tries to put it out of bypass mode,
>> > hence the warning.
>> >
>> > So either of these should work, I have not tested it, just coded it up.
>>
>> I'm pretty sure that both of your solutions are wrong.
>>
>> Jens and James, it appears the problem is in blk_register_queue().  The
>> code does this:
>>
>>   /*
>>* Initialization must be complete by now.  Finish the initial
>>* bypass from queue allocation.
>>*/
>>   queue_flag_set_unlocked(QUEUE_FLAG_INIT_DONE, q);
>>   blk_queue_bypass_end(q);
>>
>> This doesn't work well if the queue is unregistered later and then
>> registered again -- which is what happens when the sd driver is unbound
>> from a device and then bound again.  It looks like the code should be:
>>
>>   if (!queue_flag_test_and_set(QUEUE_FLAG_INIT_DONE, q))
>>   blk_queue_bypass_end(q);
>>
>> Do you agree?  If so, I'll send in patch.
>
> This looks like a nasty hack.  In theory the QUEUE_FLAG_INIT_DONE should
> be unset on blk_unregister_queue() to match the teardown; it's only
> accident it isn't.  del_gendisk() in sd_remove() is supposed to tear a
> lot of queue stuff down.  However, the problem looks to be the mismatch
> in assumptions.  The way SCSI binding works, the queue belongs to the
> underlying device so we always assumed we could add and remove upper
> drivers ... there's even a case for this if you don't want a disk but
> want to attach sg instead.  However, it's not the common use case.
>
> The block model now seems to tie a lot of queue set up and teardown to
> add and remove of the gendisk which is counter to these assumptions.  As
> long as we can go from del->add without calling the ->release function
> on the queue, everything works.  Most of the operations seem
> symmetrical, so perhaps this is only the bypass doing too much.
>
> The ideal is that disk teardown only does as much as disk setup, so the
> mid layer can still use the underlying queue on the device.
>
> This bypass code is not very well documented.  However, your problem
> seems to be caused by this change:
>
> commit 776687bce42bb22cce48b5da950e48ebbb9a948f
> Author: Tejun Heo 
> Date:   Tue Jul 1 10:29:17 2014 -0600
>
> block, blk-mq: draining can't be skipped even if bypass_depth was
> non-zero
>
> Your hack seems to indicate that this doesn't work on the add->del->add
> transtion of a gendisk.
>
> James
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: WARNING in block layer triggered in 3.17-rc3

2014-09-08 Thread Shirish Pargaonkar

So should a request queue be in bypass mode when the device is being detached
and queue is being unregistereed because requests can get queued up?


On Mon, Sep 8, 2014 at 9:51 AM, Alan Stern  wrote:
> On Sun, 7 Sep 2014, Shirish Pargaonkar wrote:
>
>> I think the problem is, when a gendisk is detached, its request queue
>> is not put in bypass mode
>> cause when it is re-attached, code tries to put it out of bypass mode,
>> hence the warning.
>>
>> So either of these should work, I have not tested it, just coded it up.
>
> I'm pretty sure that both of your solutions are wrong.
>
> Jens and James, it appears the problem is in blk_register_queue().  The
> code does this:
>
> /*
>  * Initialization must be complete by now.  Finish the initial
>  * bypass from queue allocation.
>  */
> queue_flag_set_unlocked(QUEUE_FLAG_INIT_DONE, q);
> blk_queue_bypass_end(q);
>
> This doesn't work well if the queue is unregistered later and then
> registered again -- which is what happens when the sd driver is unbound
> from a device and then bound again.  It looks like the code should be:
>
> if (!queue_flag_test_and_set(QUEUE_FLAG_INIT_DONE, q))
> blk_queue_bypass_end(q);
>
> Do you agree?  If so, I'll send in patch.
>
> Alan Stern
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: WARNING in block layer triggered in 3.17-rc3

2014-09-08 Thread Shirish Pargaonkar

So should a request queue be in bypass mode when the device is being detached
and queue is being unregistereed because requests can get queued up?


On Mon, Sep 8, 2014 at 9:51 AM, Alan Stern st...@rowland.harvard.edu wrote:
 On Sun, 7 Sep 2014, Shirish Pargaonkar wrote:

 I think the problem is, when a gendisk is detached, its request queue
 is not put in bypass mode
 cause when it is re-attached, code tries to put it out of bypass mode,
 hence the warning.

 So either of these should work, I have not tested it, just coded it up.

 I'm pretty sure that both of your solutions are wrong.

 Jens and James, it appears the problem is in blk_register_queue().  The
 code does this:

 /*
  * Initialization must be complete by now.  Finish the initial
  * bypass from queue allocation.
  */
 queue_flag_set_unlocked(QUEUE_FLAG_INIT_DONE, q);
 blk_queue_bypass_end(q);

 This doesn't work well if the queue is unregistered later and then
 registered again -- which is what happens when the sd driver is unbound
 from a device and then bound again.  It looks like the code should be:

 if (!queue_flag_test_and_set(QUEUE_FLAG_INIT_DONE, q))
 blk_queue_bypass_end(q);

 Do you agree?  If so, I'll send in patch.

 Alan Stern

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: WARNING in block layer triggered in 3.17-rc3

2014-09-08 Thread Shirish Pargaonkar

see this issue/warning in 3.12 also.

On Mon, Sep 8, 2014 at 10:51 AM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
 On Mon, 2014-09-08 at 10:51 -0400, Alan Stern wrote:
 On Sun, 7 Sep 2014, Shirish Pargaonkar wrote:

  I think the problem is, when a gendisk is detached, its request queue
  is not put in bypass mode
  cause when it is re-attached, code tries to put it out of bypass mode,
  hence the warning.
 
  So either of these should work, I have not tested it, just coded it up.

 I'm pretty sure that both of your solutions are wrong.

 Jens and James, it appears the problem is in blk_register_queue().  The
 code does this:

   /*
* Initialization must be complete by now.  Finish the initial
* bypass from queue allocation.
*/
   queue_flag_set_unlocked(QUEUE_FLAG_INIT_DONE, q);
   blk_queue_bypass_end(q);

 This doesn't work well if the queue is unregistered later and then
 registered again -- which is what happens when the sd driver is unbound
 from a device and then bound again.  It looks like the code should be:

   if (!queue_flag_test_and_set(QUEUE_FLAG_INIT_DONE, q))
   blk_queue_bypass_end(q);

 Do you agree?  If so, I'll send in patch.

 This looks like a nasty hack.  In theory the QUEUE_FLAG_INIT_DONE should
 be unset on blk_unregister_queue() to match the teardown; it's only
 accident it isn't.  del_gendisk() in sd_remove() is supposed to tear a
 lot of queue stuff down.  However, the problem looks to be the mismatch
 in assumptions.  The way SCSI binding works, the queue belongs to the
 underlying device so we always assumed we could add and remove upper
 drivers ... there's even a case for this if you don't want a disk but
 want to attach sg instead.  However, it's not the common use case.

 The block model now seems to tie a lot of queue set up and teardown to
 add and remove of the gendisk which is counter to these assumptions.  As
 long as we can go from del-add without calling the -release function
 on the queue, everything works.  Most of the operations seem
 symmetrical, so perhaps this is only the bypass doing too much.

 The ideal is that disk teardown only does as much as disk setup, so the
 mid layer can still use the underlying queue on the device.

 This bypass code is not very well documented.  However, your problem
 seems to be caused by this change:

 commit 776687bce42bb22cce48b5da950e48ebbb9a948f
 Author: Tejun Heo t...@kernel.org
 Date:   Tue Jul 1 10:29:17 2014 -0600

 block, blk-mq: draining can't be skipped even if bypass_depth was
 non-zero

 Your hack seems to indicate that this doesn't work on the add-del-add
 transtion of a gendisk.

 James



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: WARNING in block layer triggered in 3.17-rc3

2014-09-07 Thread Shirish Pargaonkar

I think the problem is, when a gendisk is detached, its request queue
is not put in bypass mode
cause when it is re-attached, code tries to put it out of bypass mode,
hence the warning.

So either of these should work, I have not tested it, just coded it up.

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 4db5abf..f94c7ba 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -600,6 +600,8 @@ void blk_unregister_queue(struct gendisk *disk)
  if (q->request_fn)
  elv_unregister_queue(q);

+ blk_queue_bypss_start(q);
+
  kobject_uevent(>kobj, KOBJ_REMOVE);
  kobject_del(>kobj);
  blk_trace_remove_sysfs(disk_to_dev(disk));





diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 2c2041c..1c7ef55f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3054,15 +3054,26 @@ static int sd_probe(struct device *dev)
 static int sd_remove(struct device *dev)
 {
  struct scsi_disk *sdkp;
+ struct request_queue *q;
+ struct scsi_device *sdp;
  dev_t devt;

  sdkp = dev_get_drvdata(dev);
+ q = sdkp->disk->queue;
  devt = disk_devt(sdkp->disk);
  scsi_autopm_get_device(sdkp->device);

  async_synchronize_full_domain(_sd_pm_domain);
  async_synchronize_full_domain(_sd_probe_domain);
  device_del(>dev);
+
+ if (q) {
+ spin_lock_irq(q->queue_lock);
+ q->bypass_depth++;
+ queue_flag_set(QUEUE_FLAG_BYPASS, q);
+ spin_unlock_irq(q->queue_lock);
+ }
+
  del_gendisk(sdkp->disk);
  sd_shutdown(dev);



On Fri, Sep 5, 2014 at 12:45 PM, Alan Stern  wrote:
> James and Jens:
>
> I got a WARNING when unbinding the sd driver from a USB flash drive and
> then binding it back again.  Here's where the flash drive gets probed
> initially:
>
> [  143.300886] usb-storage 4-8:1.0: usb_probe_interface
> [  143.300911] usb-storage 4-8:1.0: usb_probe_interface - got id
> [  143.300930] usb-storage 4-8:1.0: USB Mass Storage device detected
> [  143.318239] scsi host0: usb-storage 4-8:1.0
> [  143.359979] scsi 0:0:0:0: Direct-Access Ut165USB2FlashStorage 0.00 
> PQ: 0 ANSI: 2
> [  143.376366] usbcore: registered new interface driver usb-storage
> [  143.468464] sd 0:0:0:0: [sda] 7892040 512-byte logical blocks: (4.04 
> GB/3.76 GiB)
> [  143.481725] sd 0:0:0:0: [sda] Write Protect is off
> [  143.485712] sd 0:0:0:0: [sda] Mode Sense: 00 00 00 00
> [  143.487064] sd 0:0:0:0: [sda] Asking for cache data failed
> [  143.498428] sd 0:0:0:0: [sda] Assuming drive cache: write through
> [  143.656797]  sda: sda1
> [  143.676922] sd 0:0:0:0: [sda] Attached SCSI removable disk
>
> Then I did
>
> echo 0:0:0:0 >/sys/bus/scsi/drivers/sd_mod/unbind
>
> followed by
>
> echo 0:0:0:0 >/sys/bus/scsi/drivers/sd_mod/bind
>
> which resulted in:
>
> [  165.079557] sd 0:0:0:0: [sda] 7892040 512-byte logical blocks: (4.04 
> GB/3.76 GiB)
> [  165.093510] sd 0:0:0:0: [sda] Write Protect is off
> [  165.104388] sd 0:0:0:0: [sda] Mode Sense: 00 00 00 00
> [  165.105632] sd 0:0:0:0: [sda] Asking for cache data failed
> [  165.115136] sd 0:0:0:0: [sda] Assuming drive cache: write through
> [  165.142950]  sda: sda1
> [  165.156480] [ cut here ]
> [  165.159912] WARNING: CPU: 0 PID: 29 at block/blk-core.c:473 
> blk_queue_bypass_end+0x4d/0x62()
> [  165.160030] Modules linked in: sd_mod usb_storage scsi_mod hid_generic 
> usbhid hid pcspkr evdev i915 cfbfillrect cfbimgblt i2c_algo_bit cfbcopyarea 
> video fbcon backlight bitblit softcursor font ehci_pci drm_kms_helper 
> uhci_hcd ehci_hcd ohci_pci ohci_hcd drm i2ccore usbcore e100 mii usb_common 
> fb fbdev fan processor button thermal_sys
> [  165.160030] CPU: 0 PID: 29 Comm: kworker/u4:1 Not tainted 
> 3.17.0-rc3AS-dirty #12
> [  165.160030] Hardware name: Hewlett-Packard HP dx2000 MT (EE004AA)/08FCh, 
> BIOS 1.1711/24/2005
> [  165.160030] Workqueue: events_unbound async_run_entry_fn
> [  165.160030]  c105d2ef  ea569e10 c12771c0  ea569e28 
> c102c5fb c114907d
> [  165.160030]  ecc18000 ea619c20 ecc18000 ea569e38 c102c676 0009 
>  ea569e44
> [  165.160030]  c114907d ea619c20 ea569e5c c114b97d ea569e5c ea619c20 
> ed74e400 ea619c2c
> [  165.160030] Call Trace:
> [  165.160030]  [] ? console_unlock+0x37e/0x3ab
> [  165.160030]  [] dump_stack+0x49/0x73
> [  165.160030]  [] warn_slowpath_common+0x5c/0x73
> [  165.160030]  [] ? blk_queue_bypass_end+0x4d/0x62
> [  165.160030]  [] warn_slowpath_null+0xf/0x13
> [  165.160030]  [] blk_queue_bypass_end+0x4d/0x62
> [  165.160030]  [] blk_register_queue+0x8f/0xc4
> [  165.160030]  [] add_disk+0x2bc/0x3a8
> [  165.160030]  [] sd_probe_async+0xf5/0x17b [sd_mod]
> [  165.160030]  [] async_run_entry_fn+0x59/0xf9
> [  165.160030]  [] process_one_work+0x187/0x2ac
> [  165.160030]  [] ? process_one_work+0x129/0x2ac
> [  165.160030]  [] worker_thread+0x1b1/0x26b
> [  165.160030]  [] ? process_scheduled_works+0x21/0x21
> [  165.160030]  [] kthread+0x82/0x87
> [  165.160030]  [] ? _raw_spin_unlock_irq+0x22/0x3f
> [  165.160030]  [] ? radix_tree_tag_set+0x3f/0xa5
> [  165.160030]  []

Re: WARNING in block layer triggered in 3.17-rc3

2014-09-07 Thread Shirish Pargaonkar

I think the problem is, when a gendisk is detached, its request queue
is not put in bypass mode
cause when it is re-attached, code tries to put it out of bypass mode,
hence the warning.

So either of these should work, I have not tested it, just coded it up.

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 4db5abf..f94c7ba 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -600,6 +600,8 @@ void blk_unregister_queue(struct gendisk *disk)
  if (q-request_fn)
  elv_unregister_queue(q);

+ blk_queue_bypss_start(q);
+
  kobject_uevent(q-kobj, KOBJ_REMOVE);
  kobject_del(q-kobj);
  blk_trace_remove_sysfs(disk_to_dev(disk));





diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 2c2041c..1c7ef55f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3054,15 +3054,26 @@ static int sd_probe(struct device *dev)
 static int sd_remove(struct device *dev)
 {
  struct scsi_disk *sdkp;
+ struct request_queue *q;
+ struct scsi_device *sdp;
  dev_t devt;

  sdkp = dev_get_drvdata(dev);
+ q = sdkp-disk-queue;
  devt = disk_devt(sdkp-disk);
  scsi_autopm_get_device(sdkp-device);

  async_synchronize_full_domain(scsi_sd_pm_domain);
  async_synchronize_full_domain(scsi_sd_probe_domain);
  device_del(sdkp-dev);
+
+ if (q) {
+ spin_lock_irq(q-queue_lock);
+ q-bypass_depth++;
+ queue_flag_set(QUEUE_FLAG_BYPASS, q);
+ spin_unlock_irq(q-queue_lock);
+ }
+
  del_gendisk(sdkp-disk);
  sd_shutdown(dev);



On Fri, Sep 5, 2014 at 12:45 PM, Alan Stern st...@rowland.harvard.edu wrote:
 James and Jens:

 I got a WARNING when unbinding the sd driver from a USB flash drive and
 then binding it back again.  Here's where the flash drive gets probed
 initially:

 [  143.300886] usb-storage 4-8:1.0: usb_probe_interface
 [  143.300911] usb-storage 4-8:1.0: usb_probe_interface - got id
 [  143.300930] usb-storage 4-8:1.0: USB Mass Storage device detected
 [  143.318239] scsi host0: usb-storage 4-8:1.0
 [  143.359979] scsi 0:0:0:0: Direct-Access Ut165USB2FlashStorage 0.00 
 PQ: 0 ANSI: 2
 [  143.376366] usbcore: registered new interface driver usb-storage
 [  143.468464] sd 0:0:0:0: [sda] 7892040 512-byte logical blocks: (4.04 
 GB/3.76 GiB)
 [  143.481725] sd 0:0:0:0: [sda] Write Protect is off
 [  143.485712] sd 0:0:0:0: [sda] Mode Sense: 00 00 00 00
 [  143.487064] sd 0:0:0:0: [sda] Asking for cache data failed
 [  143.498428] sd 0:0:0:0: [sda] Assuming drive cache: write through
 [  143.656797]  sda: sda1
 [  143.676922] sd 0:0:0:0: [sda] Attached SCSI removable disk

 Then I did

 echo 0:0:0:0 /sys/bus/scsi/drivers/sd_mod/unbind

 followed by

 echo 0:0:0:0 /sys/bus/scsi/drivers/sd_mod/bind

 which resulted in:

 [  165.079557] sd 0:0:0:0: [sda] 7892040 512-byte logical blocks: (4.04 
 GB/3.76 GiB)
 [  165.093510] sd 0:0:0:0: [sda] Write Protect is off
 [  165.104388] sd 0:0:0:0: [sda] Mode Sense: 00 00 00 00
 [  165.105632] sd 0:0:0:0: [sda] Asking for cache data failed
 [  165.115136] sd 0:0:0:0: [sda] Assuming drive cache: write through
 [  165.142950]  sda: sda1
 [  165.156480] [ cut here ]
 [  165.159912] WARNING: CPU: 0 PID: 29 at block/blk-core.c:473 
 blk_queue_bypass_end+0x4d/0x62()
 [  165.160030] Modules linked in: sd_mod usb_storage scsi_mod hid_generic 
 usbhid hid pcspkr evdev i915 cfbfillrect cfbimgblt i2c_algo_bit cfbcopyarea 
 video fbcon backlight bitblit softcursor font ehci_pci drm_kms_helper 
 uhci_hcd ehci_hcd ohci_pci ohci_hcd drm i2ccore usbcore e100 mii usb_common 
 fb fbdev fan processor button thermal_sys
 [  165.160030] CPU: 0 PID: 29 Comm: kworker/u4:1 Not tainted 
 3.17.0-rc3AS-dirty #12
 [  165.160030] Hardware name: Hewlett-Packard HP dx2000 MT (EE004AA)/08FCh, 
 BIOS 1.1711/24/2005
 [  165.160030] Workqueue: events_unbound async_run_entry_fn
 [  165.160030]  c105d2ef  ea569e10 c12771c0  ea569e28 
 c102c5fb c114907d
 [  165.160030]  ecc18000 ea619c20 ecc18000 ea569e38 c102c676 0009 
  ea569e44
 [  165.160030]  c114907d ea619c20 ea569e5c c114b97d ea569e5c ea619c20 
 ed74e400 ea619c2c
 [  165.160030] Call Trace:
 [  165.160030]  [c105d2ef] ? console_unlock+0x37e/0x3ab
 [  165.160030]  [c12771c0] dump_stack+0x49/0x73
 [  165.160030]  [c102c5fb] warn_slowpath_common+0x5c/0x73
 [  165.160030]  [c114907d] ? blk_queue_bypass_end+0x4d/0x62
 [  165.160030]  [c102c676] warn_slowpath_null+0xf/0x13
 [  165.160030]  [c114907d] blk_queue_bypass_end+0x4d/0x62
 [  165.160030]  [c114b97d] blk_register_queue+0x8f/0xc4
 [  165.160030]  [c11548a6] add_disk+0x2bc/0x3a8
 [  165.160030]  [efff40a5] sd_probe_async+0xf5/0x17b [sd_mod]
 [  165.160030]  [c103fc08] async_run_entry_fn+0x59/0xf9
 [  165.160030]  [c103ab22] process_one_work+0x187/0x2ac
 [  165.160030]  [c103aac4] ? process_one_work+0x129/0x2ac
 [  165.160030]  [c103ae19] worker_thread+0x1b1/0x26b
 [  165.160030]  [c103ac68] ? process_scheduled_works+0x21/0x21
 [  165.160030]  [c103e4ee] kthread+0x82/0x87
 [  165.160030]  [c127b074] ? _raw_spin_unlock_irq+0x22/0x3f
 [

Re: [PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy

2014-08-03 Thread Shirish Pargaonkar

Joe, Thanks for watching out.

I think this patch is not correct and think also the current LM
authentication code is broken
if the password length exceeds 14 characters.

Reading from Chris Hertel's book (Chapter 15, 15.3.3 Creating the LM
Hash), it says:

The LM Hash is a sixteen byte string, created as follows:

1. The password, as entered by the user, is either padded with nuls
(0x00) or trimmed
to fourteen (14) bytes.
* Note that the 14-byte password string is not handled as
null-terminated string.  If the
  user enters 14 or more bytes, the last byte in the modified
string will not be nul.
* Also note the password is given in the 8-bit OEM character set
(extended ASCII),
  not Unicode.
2. The 14-byte password string is converted to all uppercase.

and so on

So I thought a (quick) patch like this should be a fix instead but it
does not work against a
Samba server if the strlen(password) is more than 14.

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 4934347..aecb4a0 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -307,7 +307,8 @@ int calc_lanman_hash(const char *password, const char *crypt

memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
if (password)
-   strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
+   strncpy(password_with_pad, password,
+   min_t(unsigned int, strlen(password), 14));

if (!encrypt && global_secflags & CIFSSEC_MAY_PLNTXT) {
memcpy(lnm_session_key, password_with_pad,

So there is work to be done in lanman auth code but this patch should
not be included.
So taking back the ack.

On Sat, Aug 2, 2014 at 7:27 PM, Joe Perches  wrote:
> On Sun, 2014-08-03 at 01:13 +0200, Rickard Strandqvist wrote:
>> 2014-08-02 19:33 GMT+02:00 Joe Perches :
>> > On Sat, 2014-08-02 at 11:55 -0500, Shirish Pargaonkar wrote:
>> >> Acked-by: Shirish Pargaonkar 
>> > []
>> >> > diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
>> > []
>> >> > @@ -307,7 +307,7 @@ int calc_lanman_hash(const char *password, const 
>> >> > char *cryptkey, bool encrypt,
>> >> >
>> >> > memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
>> >> > if (password)
>> >> > -   strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
>> >> > +   strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE - 
>> >> > 1);
>> >
>> >
>> > Is this always correct?
> []
>> Because password_with_pad gets set to all zeros above, the character,
>> I do not guarantee a copy terminating null.
>> Unless it is so that you do not want any terminating null.
>
> That's the question for Steve and cifs people.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy

2014-08-03 Thread Shirish Pargaonkar

Joe, Thanks for watching out.

I think this patch is not correct and think also the current LM
authentication code is broken
if the password length exceeds 14 characters.

Reading from Chris Hertel's book (Chapter 15, 15.3.3 Creating the LM
Hash), it says:

The LM Hash is a sixteen byte string, created as follows:

1. The password, as entered by the user, is either padded with nuls
(0x00) or trimmed
to fourteen (14) bytes.
* Note that the 14-byte password string is not handled as
null-terminated string.  If the
  user enters 14 or more bytes, the last byte in the modified
string will not be nul.
* Also note the password is given in the 8-bit OEM character set
(extended ASCII),
  not Unicode.
2. The 14-byte password string is converted to all uppercase.

and so on


So I thought a (quick) patch like this should be a fix instead but it
does not work against a
Samba server if the strlen(password) is more than 14.

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 4934347..aecb4a0 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -307,7 +307,8 @@ int calc_lanman_hash(const char *password, const char *crypt

memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
if (password)
-   strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
+   strncpy(password_with_pad, password,
+   min_t(unsigned int, strlen(password), 14));

if (!encrypt  global_secflags  CIFSSEC_MAY_PLNTXT) {
memcpy(lnm_session_key, password_with_pad,

So there is work to be done in lanman auth code but this patch should
not be included.
So taking back the ack.


On Sat, Aug 2, 2014 at 7:27 PM, Joe Perches j...@perches.com wrote:
 On Sun, 2014-08-03 at 01:13 +0200, Rickard Strandqvist wrote:
 2014-08-02 19:33 GMT+02:00 Joe Perches j...@perches.com:
  On Sat, 2014-08-02 at 11:55 -0500, Shirish Pargaonkar wrote:
  Acked-by: Shirish Pargaonkar spargaon...@suse.com
  []
   diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
  []
   @@ -307,7 +307,7 @@ int calc_lanman_hash(const char *password, const 
   char *cryptkey, bool encrypt,
  
   memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
   if (password)
   -   strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
   +   strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE - 
   1);
 
 
  Is this always correct?
 []
 Because password_with_pad gets set to all zeros above, the character,
 I do not guarantee a copy terminating null.
 Unless it is so that you do not want any terminating null.

 That's the question for Steve and cifs people.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy

2014-08-02 Thread Shirish Pargaonkar

Acked-by: Shirish Pargaonkar 

On Sat, Jul 26, 2014 at 5:28 PM, Rickard Strandqvist
 wrote:
> If you are going to use memset before strncpy you must copy sizeof -1
>
> Signed-off-by: Rickard Strandqvist 
> ---
>  fs/cifs/cifsencrypt.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> index 4934347..dcca8b3 100644
> --- a/fs/cifs/cifsencrypt.c
> +++ b/fs/cifs/cifsencrypt.c
> @@ -307,7 +307,7 @@ int calc_lanman_hash(const char *password, const char 
> *cryptkey, bool encrypt,
>
> memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
> if (password)
> -   strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
> +   strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE - 1);
>
> if (!encrypt && global_secflags & CIFSSEC_MAY_PLNTXT) {
> memcpy(lnm_session_key, password_with_pad,
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy

2014-08-02 Thread Shirish Pargaonkar

Acked-by: Shirish Pargaonkar spargaon...@suse.com

On Sat, Jul 26, 2014 at 5:28 PM, Rickard Strandqvist
rickard_strandqv...@spectrumdigital.se wrote:
 If you are going to use memset before strncpy you must copy sizeof -1

 Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
 ---
  fs/cifs/cifsencrypt.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
 index 4934347..dcca8b3 100644
 --- a/fs/cifs/cifsencrypt.c
 +++ b/fs/cifs/cifsencrypt.c
 @@ -307,7 +307,7 @@ int calc_lanman_hash(const char *password, const char 
 *cryptkey, bool encrypt,

 memset(password_with_pad, 0, CIFS_ENCPWD_SIZE);
 if (password)
 -   strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE);
 +   strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE - 1);

 if (!encrypt  global_secflags  CIFSSEC_MAY_PLNTXT) {
 memcpy(lnm_session_key, password_with_pad,
 --
 1.7.10.4

 --
 To unsubscribe from this list: send the line unsubscribe linux-cifs in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH block/for-linus] blkcg: don't call into policy draining if root_blkg is already gone

2014-07-12 Thread Shirish Pargaonkar

On Sat, 2014-07-05 at 18:43 -0400, Tejun Heo wrote:
> While a queue is being destroyed, all the blkgs are destroyed and its
> ->root_blkg pointer is set to NULL.  If someone else starts to drain
> while the queue is in this state, the following oops happens.
> 
>   NULL pointer dereference at 0028
>   IP: [] blk_throtl_drain+0x84/0x230
>   PGD e4a1067 PUD b773067 PMD 0
>   Oops:  [#1] PREEMPT SMP DEBUG_PAGEALLOC
>   Modules linked in: cfq_iosched(-) [last unloaded: cfq_iosched]
>   CPU: 1 PID: 537 Comm: bash Not tainted 3.16.0-rc3-work+ #2
>   Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
>   task: 88000e50 ti: 88000efd4000 task.ti: 88000efd4000
>   RIP: 0010:[]  [] 
> blk_throtl_drain+0x84/0x230
>   RSP: 0018:88000efd7bf0  EFLAGS: 00010046
>   RAX:  RBX: 880015091450 RCX: 0001
>   RDX:  RSI:  RDI: 
>   RBP: 88000efd7c10 R08:  R09: 0001
>   R10: 88000e50 R11:  R12: 880015091450
>   R13: 880015092e00 R14: 880015091d70 R15: 88001508fc28
>   FS:  7f1332650740() GS:88001fa8() knlGS:
>   CS:  0010 DS:  ES:  CR0: 8005003b
>   CR2: 0028 CR3: 09446000 CR4: 06e0
>   Stack:
>8144e8f6 880015091450  880015091d80
>88000efd7c28 8144ae2f 880015091450 88000efd7c58
>81427641 880015091450 82401f00 880015091450
>   Call Trace:
>[] blkcg_drain_queue+0x1f/0x60
>[] __blk_drain_queue+0x71/0x180
>[] blk_queue_bypass_start+0x6e/0xb0
>[] blkcg_deactivate_policy+0x38/0x120
>[] blk_throtl_exit+0x34/0x50
>[] blkcg_exit_queue+0x35/0x40
>[] blk_release_queue+0x26/0xd0
>[] kobject_cleanup+0x38/0x70
>[] kobject_put+0x28/0x60
>[] blk_put_queue+0x15/0x20
>[] scsi_device_dev_release_usercontext+0x16b/0x1c0
>[] execute_in_process_context+0x89/0xa0
>[] scsi_device_dev_release+0x1c/0x20
>[] device_release+0x32/0xa0
>[] kobject_cleanup+0x38/0x70
>[] kobject_put+0x28/0x60
>[] put_device+0x17/0x20
>[] __scsi_remove_device+0xa9/0xe0
>[] scsi_remove_device+0x2b/0x40
>[] sdev_store_delete+0x27/0x30
>[] dev_attr_store+0x18/0x30
>[] sysfs_kf_write+0x3e/0x50
>[] kernfs_fop_write+0xe7/0x170
>[] vfs_write+0xaf/0x1d0
>[] SyS_write+0x4d/0xc0
>[] system_call_fastpath+0x16/0x1b
> 
> 776687bce42b ("block, blk-mq: draining can't be skipped even if
> bypass_depth was non-zero") made it easier to trigger this bug by
> making blk_queue_bypass_start() drain even when it loses the first
> bypass test to blk_cleanup_queue(); however, the bug has always been
> there even before the commit as blk_queue_bypass_start() could race
> against queue destruction, win the initial bypass test but perform the
> actual draining after blk_cleanup_queue() already destroyed all blkgs.
> 
> Fix it by skippping calling into policy draining if all the blkgs are
> already gone.
> 
> Signed-off-by: Tejun Heo 
> Reported-by: Shirish Pargaonkar 
> Reported-by: Sasha Levin 
> Reported-by: Jet Chen 
> Cc: sta...@vger.kernel.org
> ---
>  block/blk-cgroup.c |7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index b9f4cc4..28d227c 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -872,6 +872,13 @@ void blkcg_drain_queue(struct request_queue *q)
>  {
>   lockdep_assert_held(q->queue_lock);
>  
> +     /*
> +  * @q could be exiting and already have destroyed all blkgs as
> +  * indicated by NULL root_blkg.  If so, don't confuse policies.
> +  */
> + if (!q->root_blkg)
> + return;
> +
>   blk_throtl_drain(q);
>  }
>  
> 

Tested-by: Shirish Pargaonkar 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH block/for-linus] blkcg: don't call into policy draining if root_blkg is already gone

2014-07-12 Thread Shirish Pargaonkar

On Sat, 2014-07-05 at 18:43 -0400, Tejun Heo wrote:
 While a queue is being destroyed, all the blkgs are destroyed and its
 -root_blkg pointer is set to NULL.  If someone else starts to drain
 while the queue is in this state, the following oops happens.
 
   NULL pointer dereference at 0028
   IP: [8144e944] blk_throtl_drain+0x84/0x230
   PGD e4a1067 PUD b773067 PMD 0
   Oops:  [#1] PREEMPT SMP DEBUG_PAGEALLOC
   Modules linked in: cfq_iosched(-) [last unloaded: cfq_iosched]
   CPU: 1 PID: 537 Comm: bash Not tainted 3.16.0-rc3-work+ #2
   Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
   task: 88000e50 ti: 88000efd4000 task.ti: 88000efd4000
   RIP: 0010:[8144e944]  [8144e944] 
 blk_throtl_drain+0x84/0x230
   RSP: 0018:88000efd7bf0  EFLAGS: 00010046
   RAX:  RBX: 880015091450 RCX: 0001
   RDX:  RSI:  RDI: 
   RBP: 88000efd7c10 R08:  R09: 0001
   R10: 88000e50 R11:  R12: 880015091450
   R13: 880015092e00 R14: 880015091d70 R15: 88001508fc28
   FS:  7f1332650740() GS:88001fa8() knlGS:
   CS:  0010 DS:  ES:  CR0: 8005003b
   CR2: 0028 CR3: 09446000 CR4: 06e0
   Stack:
8144e8f6 880015091450  880015091d80
88000efd7c28 8144ae2f 880015091450 88000efd7c58
81427641 880015091450 82401f00 880015091450
   Call Trace:
[8144ae2f] blkcg_drain_queue+0x1f/0x60
[81427641] __blk_drain_queue+0x71/0x180
[81429b3e] blk_queue_bypass_start+0x6e/0xb0
[814498b8] blkcg_deactivate_policy+0x38/0x120
[8144ec44] blk_throtl_exit+0x34/0x50
[8144aea5] blkcg_exit_queue+0x35/0x40
[8142d476] blk_release_queue+0x26/0xd0
[81454968] kobject_cleanup+0x38/0x70
[81454848] kobject_put+0x28/0x60
[81427505] blk_put_queue+0x15/0x20
[817d07bb] scsi_device_dev_release_usercontext+0x16b/0x1c0
[810bc339] execute_in_process_context+0x89/0xa0
[817d064c] scsi_device_dev_release+0x1c/0x20
[817930e2] device_release+0x32/0xa0
[81454968] kobject_cleanup+0x38/0x70
[81454848] kobject_put+0x28/0x60
[817934d7] put_device+0x17/0x20
[817d11b9] __scsi_remove_device+0xa9/0xe0
[817d121b] scsi_remove_device+0x2b/0x40
[817d1257] sdev_store_delete+0x27/0x30
[81792ca8] dev_attr_store+0x18/0x30
[8126f75e] sysfs_kf_write+0x3e/0x50
[8126ea87] kernfs_fop_write+0xe7/0x170
[811f5e9f] vfs_write+0xaf/0x1d0
[811f69bd] SyS_write+0x4d/0xc0
[81d24692] system_call_fastpath+0x16/0x1b
 
 776687bce42b (block, blk-mq: draining can't be skipped even if
 bypass_depth was non-zero) made it easier to trigger this bug by
 making blk_queue_bypass_start() drain even when it loses the first
 bypass test to blk_cleanup_queue(); however, the bug has always been
 there even before the commit as blk_queue_bypass_start() could race
 against queue destruction, win the initial bypass test but perform the
 actual draining after blk_cleanup_queue() already destroyed all blkgs.
 
 Fix it by skippping calling into policy draining if all the blkgs are
 already gone.
 
 Signed-off-by: Tejun Heo t...@kernel.org
 Reported-by: Shirish Pargaonkar spargaon...@suse.com
 Reported-by: Sasha Levin sasha.le...@oracle.com
 Reported-by: Jet Chen jet.c...@intel.com
 Cc: sta...@vger.kernel.org
 ---
  block/blk-cgroup.c |7 +++
  1 file changed, 7 insertions(+)
 
 diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
 index b9f4cc4..28d227c 100644
 --- a/block/blk-cgroup.c
 +++ b/block/blk-cgroup.c
 @@ -872,6 +872,13 @@ void blkcg_drain_queue(struct request_queue *q)
  {
   lockdep_assert_held(q-queue_lock);
  
 + /*
 +  * @q could be exiting and already have destroyed all blkgs as
 +  * indicated by NULL root_blkg.  If so, don't confuse policies.
 +  */
 + if (!q-root_blkg)
 + return;
 +
   blk_throtl_drain(q);
  }
  
 

Tested-by: Shirish Pargaonkar spargaon...@suse.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.

2014-07-04 Thread Shirish Pargaonkar

Tejun, I do not have another iteration of the patch yet.

we have two customers who reported this problem.  One of them has not
responded to requests to test a/any patch and other one is not able to
recreate this
with the latest release.  And I am not able to recreate this problem
on my own but
working on a setup.  So if you have any patch, I would be happy to
test/deploy it on a
first setup I can get hold of.

I was also trying to figure out whether this is some refcounting issue
or not since
I was thinking that put by anybody other than blk_cleanup_queue() should
not result on freeing/blk_release_queue() i.e. put by blk_cleanup_queue() should
always be the last put.

Regards,

Shirish


On Fri, Jul 4, 2014 at 12:18 PM, Tejun Heo  wrote:
> On Tue, Jun 24, 2014 at 05:43:40PM -0400, Tejun Heo wrote:
>> Hello,
>>
>> On Tue, Jun 24, 2014 at 04:37:58PM -0500, Shirish Pargaonkar wrote:
>> > When we start from blk_cleanup_queue(), we put request queue in bypass 
>> > mode,
>> > drain it (and service queues), and then destroy blkcgs (explicitly)
>> >
>> > When we start from blk_release_queue(), we do not drain first and then
>> > destroy blkcgs.  So if we destroy blkcg and then call (implicitly) and
>> > bail out of
>> > blk_drain_queue, we would not have drained the service queues which
>> > is not what we want.
>>
>> I'm not really following you.  What do you mean "when we start from
>> blk_release_queue()"?  blk_release_queue() is called after the last
>> put which can only follow blk_cleanup_queue() if the queue is fully
>> initialized.  The queue is already in bypass mode and fully drained by
>> the time control reaches blk_release_queue().  Module [un]load
>> re-invoking the path doesn't change anything.
>>
>> > I do not see any harm in waiting till end to release blkcgs (as I 
>> > understand).
>>
>> Well, the harm there is not freeing those blkgs unless all the blkcg
>> policies are unloaded which is usually never on most systems.
>
> Ping.  We have a patch which makes this problem more visible.  Are you
> still planning to re-spin the patch?
>
> Thanks.
>
> --
> tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.

2014-07-04 Thread Shirish Pargaonkar

Tejun, I do not have another iteration of the patch yet.

we have two customers who reported this problem.  One of them has not
responded to requests to test a/any patch and other one is not able to
recreate this
with the latest release.  And I am not able to recreate this problem
on my own but
working on a setup.  So if you have any patch, I would be happy to
test/deploy it on a
first setup I can get hold of.

I was also trying to figure out whether this is some refcounting issue
or not since
I was thinking that put by anybody other than blk_cleanup_queue() should
not result on freeing/blk_release_queue() i.e. put by blk_cleanup_queue() should
always be the last put.

Regards,

Shirish


On Fri, Jul 4, 2014 at 12:18 PM, Tejun Heo t...@kernel.org wrote:
 On Tue, Jun 24, 2014 at 05:43:40PM -0400, Tejun Heo wrote:
 Hello,

 On Tue, Jun 24, 2014 at 04:37:58PM -0500, Shirish Pargaonkar wrote:
  When we start from blk_cleanup_queue(), we put request queue in bypass 
  mode,
  drain it (and service queues), and then destroy blkcgs (explicitly)
 
  When we start from blk_release_queue(), we do not drain first and then
  destroy blkcgs.  So if we destroy blkcg and then call (implicitly) and
  bail out of
  blk_drain_queue, we would not have drained the service queues which
  is not what we want.

 I'm not really following you.  What do you mean when we start from
 blk_release_queue()?  blk_release_queue() is called after the last
 put which can only follow blk_cleanup_queue() if the queue is fully
 initialized.  The queue is already in bypass mode and fully drained by
 the time control reaches blk_release_queue().  Module [un]load
 re-invoking the path doesn't change anything.

  I do not see any harm in waiting till end to release blkcgs (as I 
  understand).

 Well, the harm there is not freeing those blkgs unless all the blkcg
 policies are unloaded which is usually never on most systems.

 Ping.  We have a patch which makes this problem more visible.  Are you
 still planning to re-spin the patch?

 Thanks.

 --
 tejun
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.

2014-06-24 Thread Shirish Pargaonkar

When we start from blk_cleanup_queue(), we put request queue in bypass mode,
drain it (and service queues), and then destroy blkcgs (explicitly)

When we start from blk_release_queue(), we do not drain first and then
destroy blkcgs.  So if we destroy blkcg and then call (implicitly) and
bail out of
blk_drain_queue, we would not have drained the service queues which
is not what we want.

I do not see any harm in waiting till end to release blkcgs (as I understand).

Regards,

Shirish

On Tue, Jun 24, 2014 at 4:09 PM, Tejun Heo  wrote:
> Hello,
>
> On Mon, Jun 23, 2014 at 01:52:25PM -0500, shirishpargaon...@gmail.com wrote:
>> From: Shirish Pargaonkar 
>>
>> Release blkg infrastructure only after last policy is deactivated
>> (i.e. let blkg_destroy_all() be called only from blkcg_deactivate_policy())
>>
>> Otherwise, module can oops because root_blkg gets assigned NULL during
>> cleanup and we attempt draining the service queues via root_blkg afterwords.
>
> I'm not sure this fix makes sense.  Cleanup path oopses on an already
> freed resource.  How can the solution be not freeing?  Why not simply
> make blkcg_drain_queue() bail if the blkgs are all destroyed?  The
> whole thing is fully synchronized with the queuelock, right?
>
> Can you please also cc Jens when you post the next iteration?
>
> Thanks.
>
> --
> tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.

2014-06-24 Thread Shirish Pargaonkar

When we start from blk_cleanup_queue(), we put request queue in bypass mode,
drain it (and service queues), and then destroy blkcgs (explicitly)

When we start from blk_release_queue(), we do not drain first and then
destroy blkcgs.  So if we destroy blkcg and then call (implicitly) and
bail out of
blk_drain_queue, we would not have drained the service queues which
is not what we want.

I do not see any harm in waiting till end to release blkcgs (as I understand).

Regards,

Shirish

On Tue, Jun 24, 2014 at 4:09 PM, Tejun Heo t...@kernel.org wrote:
 Hello,

 On Mon, Jun 23, 2014 at 01:52:25PM -0500, shirishpargaon...@gmail.com wrote:
 From: Shirish Pargaonkar spargaon...@suse.com

 Release blkg infrastructure only after last policy is deactivated
 (i.e. let blkg_destroy_all() be called only from blkcg_deactivate_policy())

 Otherwise, module can oops because root_blkg gets assigned NULL during
 cleanup and we attempt draining the service queues via root_blkg afterwords.

 I'm not sure this fix makes sense.  Cleanup path oopses on an already
 freed resource.  How can the solution be not freeing?  Why not simply
 make blkcg_drain_queue() bail if the blkgs are all destroyed?  The
 whole thing is fully synchronized with the queuelock, right?

 Can you please also cc Jens when you post the next iteration?

 Thanks.

 --
 tejun
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.

2014-06-23 Thread Shirish Pargaonkar

] c03a604c blk_put_queue+0x2c/0x50
[c002db203830] d28c55bc
scsi_device_dev_release_usercontext+0x12c/0x190 [scsi_mod]
[c002db203890] c00c29b4 execute_in_process_context+0xa4/0xd0
[c002db2038c0] d28c5474 scsi_device_dev_release+0x34/0x50
[scsi_mod]
[c002db2038f0] c04c3c10 device_release+0x60/0xf0
[c002db203970] c03e2124 kobject_cleanup+0xd4/0x240
[c002db2039f0] c04c4324 put_device+0x34/0x50
[c002db203a20] d28c66e4 __scsi_remove_device+0xf4/0x150 [scsi_mod]
[c002db203a50] d28c3844 scsi_forget_host+0xa4/0xc0 [scsi_mod]
[c002db203a80] d28b3a24 scsi_remove_host+0xa4/0x1b0 [scsi_mod]
[c002db203ac0] d2093c04 sdebug_driver_remove+0x54/0x100
[scsi_debug]
[c002db203b50] c04caa9c __device_release_driver+0xac/0x130
[c002db203b80] c04cab60 device_release_driver+0x40/0x70
[c002db203bb0] c04c9f18 bus_remove_device+0x138/0x1e0
[c002db203c30] c04c466c device_del+0x16c/0x260
[c002db203c70] c04c479c device_unregister+0x3c/0xa0
[c002db203ce0] d20934e4 sdebug_remove_adapter+0xa4/0xf0
[scsi_debug]
[c002db203d10] d2099410 scsi_debug_exit+0x3c/0x116c [scsi_debug]
[c002db203d40] c013b520 SyS_delete_module+0x200/0x350
[c002db203e30] c0009dfc syscall_exit+0x0/0x7c
--- Exception: c01 (System Call) at 3fff92974040
SP (3fffd65d4cc0) is in userspace

On Mon, Jun 23, 2014 at 1:52 PM,   wrote:
> From: Shirish Pargaonkar 
>
> Release blkg infrastructure only after last policy is deactivated
> (i.e. let blkg_destroy_all() be called only from blkcg_deactivate_policy())
>
> Otherwise, module can oops because root_blkg gets assigned NULL during
> cleanup and we attempt draining the service queues via root_blkg afterwords.
>
> Signed-off-by: Shirish Pargaonkar 
> Reported-and-tested-by: Sachin Sant 
> ---
>  block/blk-cgroup.c | 15 ---
>  block/blk-sysfs.c  |  2 +-
>  2 files changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 069bc20..3920de6 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -878,21 +878,6 @@ void blkcg_drain_queue(struct request_queue *q)
> blk_throtl_drain(q);
>  }
>
> -/**
> - * blkcg_exit_queue - exit and release blkcg part of request_queue
> - * @q: request_queue being released
> - *
> - * Called from blk_release_queue().  Responsible for exiting blkcg part.
> - */
> -void blkcg_exit_queue(struct request_queue *q)
> -{
> -   spin_lock_irq(q->queue_lock);
> -   blkg_destroy_all(q);
> -   spin_unlock_irq(q->queue_lock);
> -
> -   blk_throtl_exit(q);
> -}
> -
>  /*
>   * We cannot support shared io contexts, as we have no mean to support
>   * two tasks with the same ioc in two different groups without major rework
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 23321fb..ee24cd2 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -503,7 +503,7 @@ static void blk_release_queue(struct kobject *kobj)
>
> blk_sync_queue(q);
>
> -   blkcg_exit_queue(q);
> +   blk_throtl_exit(q);
>
> if (q->elevator) {
> spin_lock_irq(q->queue_lock);
> --
> 1.8.3.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.

2014-06-23 Thread Shirish Pargaonkar

+0xd4/0x240
[c002db203800] c03a604c blk_put_queue+0x2c/0x50
[c002db203830] d28c55bc
scsi_device_dev_release_usercontext+0x12c/0x190 [scsi_mod]
[c002db203890] c00c29b4 execute_in_process_context+0xa4/0xd0
[c002db2038c0] d28c5474 scsi_device_dev_release+0x34/0x50
[scsi_mod]
[c002db2038f0] c04c3c10 device_release+0x60/0xf0
[c002db203970] c03e2124 kobject_cleanup+0xd4/0x240
[c002db2039f0] c04c4324 put_device+0x34/0x50
[c002db203a20] d28c66e4 __scsi_remove_device+0xf4/0x150 [scsi_mod]
[c002db203a50] d28c3844 scsi_forget_host+0xa4/0xc0 [scsi_mod]
[c002db203a80] d28b3a24 scsi_remove_host+0xa4/0x1b0 [scsi_mod]
[c002db203ac0] d2093c04 sdebug_driver_remove+0x54/0x100
[scsi_debug]
[c002db203b50] c04caa9c __device_release_driver+0xac/0x130
[c002db203b80] c04cab60 device_release_driver+0x40/0x70
[c002db203bb0] c04c9f18 bus_remove_device+0x138/0x1e0
[c002db203c30] c04c466c device_del+0x16c/0x260
[c002db203c70] c04c479c device_unregister+0x3c/0xa0
[c002db203ce0] d20934e4 sdebug_remove_adapter+0xa4/0xf0
[scsi_debug]
[c002db203d10] d2099410 scsi_debug_exit+0x3c/0x116c [scsi_debug]
[c002db203d40] c013b520 SyS_delete_module+0x200/0x350
[c002db203e30] c0009dfc syscall_exit+0x0/0x7c
--- Exception: c01 (System Call) at 3fff92974040
SP (3fffd65d4cc0) is in userspace

On Mon, Jun 23, 2014 at 1:52 PM,  shirishpargaon...@gmail.com wrote:
 From: Shirish Pargaonkar spargaon...@suse.com

 Release blkg infrastructure only after last policy is deactivated
 (i.e. let blkg_destroy_all() be called only from blkcg_deactivate_policy())

 Otherwise, module can oops because root_blkg gets assigned NULL during
 cleanup and we attempt draining the service queues via root_blkg afterwords.

 Signed-off-by: Shirish Pargaonkar spargaon...@suse.com
 Reported-and-tested-by: Sachin Sant ss...@in.ibm.com
 ---
  block/blk-cgroup.c | 15 ---
  block/blk-sysfs.c  |  2 +-
  2 files changed, 1 insertion(+), 16 deletions(-)

 diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
 index 069bc20..3920de6 100644
 --- a/block/blk-cgroup.c
 +++ b/block/blk-cgroup.c
 @@ -878,21 +878,6 @@ void blkcg_drain_queue(struct request_queue *q)
 blk_throtl_drain(q);
  }

 -/**
 - * blkcg_exit_queue - exit and release blkcg part of request_queue
 - * @q: request_queue being released
 - *
 - * Called from blk_release_queue().  Responsible for exiting blkcg part.
 - */
 -void blkcg_exit_queue(struct request_queue *q)
 -{
 -   spin_lock_irq(q-queue_lock);
 -   blkg_destroy_all(q);
 -   spin_unlock_irq(q-queue_lock);
 -
 -   blk_throtl_exit(q);
 -}
 -
  /*
   * We cannot support shared io contexts, as we have no mean to support
   * two tasks with the same ioc in two different groups without major rework
 diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
 index 23321fb..ee24cd2 100644
 --- a/block/blk-sysfs.c
 +++ b/block/blk-sysfs.c
 @@ -503,7 +503,7 @@ static void blk_release_queue(struct kobject *kobj)

 blk_sync_queue(q);

 -   blkcg_exit_queue(q);
 +   blk_throtl_exit(q);

 if (q-elevator) {
 spin_lock_irq(q-queue_lock);
 --
 1.8.3.2

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] cifs: Fix possible deadlock with cifs and work queues

2014-03-22 Thread Shirish Pargaonkar

Just a thought, although leasing code does not cause such deadlocks with rwsems,
perhaps leasing and oplock code can reside on one workqueue!


On Fri, Mar 21, 2014 at 10:07 AM, Steven Rostedt  wrote:
>
> We just had a customer report a deadlock in their 3.8-rt kernel.
> Looking into this, it is very possible to have the same deadlock in
> mainline in Linus's tree as it stands today.
>
> It is much easier to deadlock in the -rt kernel because reader locks
> are serialized, where a down_read() can block another down_read(). But
> because rwsems are fair locks, if a writer is waiting, a new reader
> will then block. This means that if it is possible for a reader to
> deadlock another reader, this can happen if a write comes along and
> blocks on a current reader. That will prevent another reader from
> running, and if that new reader requires to wake up a reader that owns
> the lock, you have your deadlock.
>
> Here's the situation with CIFS and workqueues:
>
> The cifs system has several workqueues used in file.c and other places.
> One of them is used for completion of a read and to release the
> page_lock which wakes up the reader. There are several other workqueues
> that do various other tasks.
>
> A holder of the reader lock can sleep on a page_lock() and expect the
> reader workqueue to wake it up (page_unlock()). The reader workqueue
> takes no locks so this does not seem to be a problem (but it is).
>
> The other workqueues can take the rwsem for read or for write. But our
> issue that we tripped over was that it grabs it for read (remember in
> -rt readers are serialized). But this can also happen if a separate
> writer is waiting on the lock as that would cause a reader to block on
> another reader too.
>
> All the workqueue callbacks are executed on the same workqueue:
>
> queue_work(cifsiod_wq, >work);
> [...]
> queue_work(cifsiod_wq, >oplock_break);
>
> Now if the reader workqueue callback is queued after one of these
> workqueues that can take the rwsem, we can hit a deadlock. The
> workqueue code looks to be able to prevent deadlocks of these kinds,
> but I do not totally understand the workqueue scheduled work structure
> and perhaps if the kworker thread structure blocks hard it wont move
> works around.
>
> Here's what we see:
>
> rdata->work is scheduled after cfile->oplock_break
>
> CPU0CPU1
> 
>
>   do_sync_read()
> cifs_strict_readv()
>   down_read(cinode->lock_sem);
>   generic_file_aio_read()
> __lock_page_killable()
>   __wait_on_bit_lock()
>
>* BLOCKED *
>
> process_one_work()
>   cifs_oplock_break()
> cifs_has_mand_locks()
>   
> down_read(cinode->lock_sem);
>
>* BLOCKED *
>
>   [ note, cifs_oplock_break() can
> also call cifs_push_locks 
> which takes
> the lock with down_write() ]
>
> The key to remember is that the work to wake up the lock owner is queued
> behind the oplock work which is blocked on the same lock.
>
> We noticed that the rdata->work was queued to run under the same
> workqueue task and this work is to wake up the owner of the semaphore.
> But because the workqueue task is blocked waiting on that lock, it will
> never wake it up.
>
> By adding another workqueue that runs all the work that might take a mutex
> we should be able to avoid this deadlock.
>
> Link: http://lkml.kernel.org/r/20140319151252.16ed3...@gandalf.local.home
>
> Signed-off-by: Steven Rostedt 
> ---
>  fs/cifs/cifsfs.c   | 17 -
>  fs/cifs/cifsglob.h |  1 +
>  fs/cifs/misc.c |  2 +-
>  fs/cifs/smb2misc.c |  6 +++---
>  4 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 849f613..b0761c8 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -86,6 +86,12 @@ extern mempool_t *cifs_req_poolp;
>  extern mempool_t *cifs_mid_poolp;
>
>  struct workqueue_struct*cifsiod_wq;
> +/*
> + * The oplock workqueue must be separate to prevent it from blocking
> + * other work that is queued. Work that requires to grab mutex locks
> + * must use the 'l' version.
> + */
> +struct workqueue_struct*cifsiold_wq;
>
>  #ifdef CONFIG_CIFS_SMB2
>  __u8 cifs_client_guid[SMB2_CLIENT_GUID_SIZE];
> @@ -1199,9 +1205,15 @@ init_cifs(void)
> goto out_clean_proc;
> }
>
> +   cifsiold_wq = alloc_workqueue("cifsiold", 
> WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
> +   if (!cifsiold_wq) {
> +   rc = -ENOMEM;
> +   goto

Re: [PATCH v2] cifs: Fix possible deadlock with cifs and work queues

2014-03-22 Thread Shirish Pargaonkar

Just a thought, although leasing code does not cause such deadlocks with rwsems,
perhaps leasing and oplock code can reside on one workqueue!


On Fri, Mar 21, 2014 at 10:07 AM, Steven Rostedt rost...@goodmis.org wrote:

 We just had a customer report a deadlock in their 3.8-rt kernel.
 Looking into this, it is very possible to have the same deadlock in
 mainline in Linus's tree as it stands today.

 It is much easier to deadlock in the -rt kernel because reader locks
 are serialized, where a down_read() can block another down_read(). But
 because rwsems are fair locks, if a writer is waiting, a new reader
 will then block. This means that if it is possible for a reader to
 deadlock another reader, this can happen if a write comes along and
 blocks on a current reader. That will prevent another reader from
 running, and if that new reader requires to wake up a reader that owns
 the lock, you have your deadlock.

 Here's the situation with CIFS and workqueues:

 The cifs system has several workqueues used in file.c and other places.
 One of them is used for completion of a read and to release the
 page_lock which wakes up the reader. There are several other workqueues
 that do various other tasks.

 A holder of the reader lock can sleep on a page_lock() and expect the
 reader workqueue to wake it up (page_unlock()). The reader workqueue
 takes no locks so this does not seem to be a problem (but it is).

 The other workqueues can take the rwsem for read or for write. But our
 issue that we tripped over was that it grabs it for read (remember in
 -rt readers are serialized). But this can also happen if a separate
 writer is waiting on the lock as that would cause a reader to block on
 another reader too.

 All the workqueue callbacks are executed on the same workqueue:

 queue_work(cifsiod_wq, rdata-work);
 [...]
 queue_work(cifsiod_wq, cfile-oplock_break);

 Now if the reader workqueue callback is queued after one of these
 workqueues that can take the rwsem, we can hit a deadlock. The
 workqueue code looks to be able to prevent deadlocks of these kinds,
 but I do not totally understand the workqueue scheduled work structure
 and perhaps if the kworker thread structure blocks hard it wont move
 works around.

 Here's what we see:

 rdata-work is scheduled after cfile-oplock_break

 CPU0CPU1
 

   do_sync_read()
 cifs_strict_readv()
   down_read(cinode-lock_sem);
   generic_file_aio_read()
 __lock_page_killable()
   __wait_on_bit_lock()

* BLOCKED *

 process_one_work()
   cifs_oplock_break()
 cifs_has_mand_locks()
   
 down_read(cinode-lock_sem);

* BLOCKED *

   [ note, cifs_oplock_break() can
 also call cifs_push_locks 
 which takes
 the lock with down_write() ]

 The key to remember is that the work to wake up the lock owner is queued
 behind the oplock work which is blocked on the same lock.

 We noticed that the rdata-work was queued to run under the same
 workqueue task and this work is to wake up the owner of the semaphore.
 But because the workqueue task is blocked waiting on that lock, it will
 never wake it up.

 By adding another workqueue that runs all the work that might take a mutex
 we should be able to avoid this deadlock.

 Link: http://lkml.kernel.org/r/20140319151252.16ed3...@gandalf.local.home

 Signed-off-by: Steven Rostedt rost...@goodmis.org
 ---
  fs/cifs/cifsfs.c   | 17 -
  fs/cifs/cifsglob.h |  1 +
  fs/cifs/misc.c |  2 +-
  fs/cifs/smb2misc.c |  6 +++---
  4 files changed, 21 insertions(+), 5 deletions(-)

 diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
 index 849f613..b0761c8 100644
 --- a/fs/cifs/cifsfs.c
 +++ b/fs/cifs/cifsfs.c
 @@ -86,6 +86,12 @@ extern mempool_t *cifs_req_poolp;
  extern mempool_t *cifs_mid_poolp;

  struct workqueue_struct*cifsiod_wq;
 +/*
 + * The oplock workqueue must be separate to prevent it from blocking
 + * other work that is queued. Work that requires to grab mutex locks
 + * must use the 'l' version.
 + */
 +struct workqueue_struct*cifsiold_wq;

  #ifdef CONFIG_CIFS_SMB2
  __u8 cifs_client_guid[SMB2_CLIENT_GUID_SIZE];
 @@ -1199,9 +1205,15 @@ init_cifs(void)
 goto out_clean_proc;
 }

 +   cifsiold_wq = alloc_workqueue(cifsiold, 
 WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
 +   if (!cifsiold_wq) {
 +   rc = -ENOMEM;
 +   goto out_destroy_wq;
 +   }
 +
 rc = cifs_fscache_register();
 if

Re: [PATCH linux-next] cifs: Use data structures to compute NTLMv2 response offsets

2013-11-08 Thread Shirish Pargaonkar

Looks correct.  You may want to verify that the code works fine for both
sec=ntlmssp/ntlmsspi and sec=ntlmv2/ntlmv2i mount options.

Reviewed-by: Shirish Pargaonkar 

On Thu, Nov 7, 2013 at 5:40 PM, Tim Gardner  wrote:
> A bit of cleanup plus some gratuitous variable renaming. I think using
> structures instead of numeric offsets makes this code much more
> understandable.
>
> Also added a comment about current time range expected by
> the server.
>
> Cc: Jeff Layton 
> Cc: Steve French 
> Signed-off-by: Tim Gardner 
> ---
>
> The comment about time of day needing to be within 5 minutes is important (to 
> me
> at least). I spent the best part of a week thinking I had endian issues on 
> powerpc
> when in truth I was just too stupid to notice that the clock
> was not updated. Danged embedded platforms...
>
> checkpatch has some problems with this patch regarding attribute packed, but 
> I chose
> to remain consistent with existing code.
>
> WARNING: __packed is preferred over __attribute__((packed))
> #141: FILE: fs/cifs/cifspdu.h:705:
> +   } __attribute__((packed)) challenge;
>
> WARNING: __packed is preferred over __attribute__((packed))
> #142: FILE: fs/cifs/cifspdu.h:706:
> +   } __attribute__((packed));
>
> total: 0 errors, 2 warnings, 99 lines checked
>
> Tested on cifs-2.6 for-linus (c481e9feee78c6ce1ba0a1c8c892049f6514f6cf) by 
> mounting
> to iOS 10.8 and Win 8.0 Pro.
>
> rtg
>
>  fs/cifs/cifsencrypt.c |   40 
>  fs/cifs/cifspdu.h |8 +++-
>  2 files changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> index fc6f4f3..4934347 100644
> --- a/fs/cifs/cifsencrypt.c
> +++ b/fs/cifs/cifsencrypt.c
> @@ -548,7 +548,13 @@ static int
>  CalcNTLMv2_response(const struct cifs_ses *ses, char *ntlmv2_hash)
>  {
> int rc;
> -   unsigned int offset = CIFS_SESS_KEY_SIZE + 8;
> +   struct ntlmv2_resp *ntlmv2 = (struct ntlmv2_resp *)
> +   (ses->auth_key.response + CIFS_SESS_KEY_SIZE);
> +   unsigned int hash_len;
> +
> +   /* The MD5 hash starts at challenge_key.key */
> +   hash_len = ses->auth_key.len - (CIFS_SESS_KEY_SIZE +
> +   offsetof(struct ntlmv2_resp, challenge.key[0]));
>
> if (!ses->server->secmech.sdeschmacmd5) {
> cifs_dbg(VFS, "%s: can't generate ntlmv2 hash\n", __func__);
> @@ -556,7 +562,7 @@ CalcNTLMv2_response(const struct cifs_ses *ses, char 
> *ntlmv2_hash)
> }
>
> rc = crypto_shash_setkey(ses->server->secmech.hmacmd5,
> -   ntlmv2_hash, CIFS_HMAC_MD5_HASH_SIZE);
> +ntlmv2_hash, CIFS_HMAC_MD5_HASH_SIZE);
> if (rc) {
> cifs_dbg(VFS, "%s: Could not set NTLMV2 Hash as a key\n",
>  __func__);
> @@ -570,20 +576,21 @@ CalcNTLMv2_response(const struct cifs_ses *ses, char 
> *ntlmv2_hash)
> }
>
> if (ses->server->negflavor == CIFS_NEGFLAVOR_EXTENDED)
> -   memcpy(ses->auth_key.response + offset,
> -   ses->ntlmssp->cryptkey, CIFS_SERVER_CHALLENGE_SIZE);
> +   memcpy(ntlmv2->challenge.key,
> +  ses->ntlmssp->cryptkey, CIFS_SERVER_CHALLENGE_SIZE);
> else
> -   memcpy(ses->auth_key.response + offset,
> -   ses->server->cryptkey, CIFS_SERVER_CHALLENGE_SIZE);
> +   memcpy(ntlmv2->challenge.key,
> +  ses->server->cryptkey, CIFS_SERVER_CHALLENGE_SIZE);
> rc = crypto_shash_update(>server->secmech.sdeschmacmd5->shash,
> -   ses->auth_key.response + offset, ses->auth_key.len - offset);
> +ntlmv2->challenge.key, hash_len);
> if (rc) {
> cifs_dbg(VFS, "%s: Could not update with response\n", 
> __func__);
> return rc;
> }
>
> +   /* Note that the MD5 digest over writes anon.challenge_key.key */
> rc = crypto_shash_final(>server->secmech.sdeschmacmd5->shash,
> -   ses->auth_key.response + CIFS_SESS_KEY_SIZE);
> +   ntlmv2->ntlmv2_hash);
> if (rc)
> cifs_dbg(VFS, "%s: Could not generate md5 hash\n", __func__);
>
> @@ -627,7 +634,7 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct 
> nls_table *nls_cp)
> int rc;
> int baselen;
> unsigned int tilen;
> -   struct ntlmv2_resp *buf;
> +   struct ntlmv2_

Re: [PATCH linux-next] cifs: Use data structures to compute NTLMv2 response offsets

2013-11-08 Thread Shirish Pargaonkar

Looks correct.  You may want to verify that the code works fine for both
sec=ntlmssp/ntlmsspi and sec=ntlmv2/ntlmv2i mount options.

Reviewed-by: Shirish Pargaonkar spargaon...@suse.com

On Thu, Nov 7, 2013 at 5:40 PM, Tim Gardner tim.gard...@canonical.com wrote:
 A bit of cleanup plus some gratuitous variable renaming. I think using
 structures instead of numeric offsets makes this code much more
 understandable.

 Also added a comment about current time range expected by
 the server.

 Cc: Jeff Layton jlay...@redhat.com
 Cc: Steve French sfre...@samba.org
 Signed-off-by: Tim Gardner tim.gard...@canonical.com
 ---

 The comment about time of day needing to be within 5 minutes is important (to 
 me
 at least). I spent the best part of a week thinking I had endian issues on 
 powerpc
 when in truth I was just too stupid to notice that the clock
 was not updated. Danged embedded platforms...

 checkpatch has some problems with this patch regarding attribute packed, but 
 I chose
 to remain consistent with existing code.

 WARNING: __packed is preferred over __attribute__((packed))
 #141: FILE: fs/cifs/cifspdu.h:705:
 +   } __attribute__((packed)) challenge;

 WARNING: __packed is preferred over __attribute__((packed))
 #142: FILE: fs/cifs/cifspdu.h:706:
 +   } __attribute__((packed));

 total: 0 errors, 2 warnings, 99 lines checked

 Tested on cifs-2.6 for-linus (c481e9feee78c6ce1ba0a1c8c892049f6514f6cf) by 
 mounting
 to iOS 10.8 and Win 8.0 Pro.

 rtg

  fs/cifs/cifsencrypt.c |   40 
  fs/cifs/cifspdu.h |8 +++-
  2 files changed, 31 insertions(+), 17 deletions(-)

 diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
 index fc6f4f3..4934347 100644
 --- a/fs/cifs/cifsencrypt.c
 +++ b/fs/cifs/cifsencrypt.c
 @@ -548,7 +548,13 @@ static int
  CalcNTLMv2_response(const struct cifs_ses *ses, char *ntlmv2_hash)
  {
 int rc;
 -   unsigned int offset = CIFS_SESS_KEY_SIZE + 8;
 +   struct ntlmv2_resp *ntlmv2 = (struct ntlmv2_resp *)
 +   (ses-auth_key.response + CIFS_SESS_KEY_SIZE);
 +   unsigned int hash_len;
 +
 +   /* The MD5 hash starts at challenge_key.key */
 +   hash_len = ses-auth_key.len - (CIFS_SESS_KEY_SIZE +
 +   offsetof(struct ntlmv2_resp, challenge.key[0]));

 if (!ses-server-secmech.sdeschmacmd5) {
 cifs_dbg(VFS, %s: can't generate ntlmv2 hash\n, __func__);
 @@ -556,7 +562,7 @@ CalcNTLMv2_response(const struct cifs_ses *ses, char 
 *ntlmv2_hash)
 }

 rc = crypto_shash_setkey(ses-server-secmech.hmacmd5,
 -   ntlmv2_hash, CIFS_HMAC_MD5_HASH_SIZE);
 +ntlmv2_hash, CIFS_HMAC_MD5_HASH_SIZE);
 if (rc) {
 cifs_dbg(VFS, %s: Could not set NTLMV2 Hash as a key\n,
  __func__);
 @@ -570,20 +576,21 @@ CalcNTLMv2_response(const struct cifs_ses *ses, char 
 *ntlmv2_hash)
 }

 if (ses-server-negflavor == CIFS_NEGFLAVOR_EXTENDED)
 -   memcpy(ses-auth_key.response + offset,
 -   ses-ntlmssp-cryptkey, CIFS_SERVER_CHALLENGE_SIZE);
 +   memcpy(ntlmv2-challenge.key,
 +  ses-ntlmssp-cryptkey, CIFS_SERVER_CHALLENGE_SIZE);
 else
 -   memcpy(ses-auth_key.response + offset,
 -   ses-server-cryptkey, CIFS_SERVER_CHALLENGE_SIZE);
 +   memcpy(ntlmv2-challenge.key,
 +  ses-server-cryptkey, CIFS_SERVER_CHALLENGE_SIZE);
 rc = crypto_shash_update(ses-server-secmech.sdeschmacmd5-shash,
 -   ses-auth_key.response + offset, ses-auth_key.len - offset);
 +ntlmv2-challenge.key, hash_len);
 if (rc) {
 cifs_dbg(VFS, %s: Could not update with response\n, 
 __func__);
 return rc;
 }

 +   /* Note that the MD5 digest over writes anon.challenge_key.key */
 rc = crypto_shash_final(ses-server-secmech.sdeschmacmd5-shash,
 -   ses-auth_key.response + CIFS_SESS_KEY_SIZE);
 +   ntlmv2-ntlmv2_hash);
 if (rc)
 cifs_dbg(VFS, %s: Could not generate md5 hash\n, __func__);

 @@ -627,7 +634,7 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct 
 nls_table *nls_cp)
 int rc;
 int baselen;
 unsigned int tilen;
 -   struct ntlmv2_resp *buf;
 +   struct ntlmv2_resp *ntlmv2;
 char ntlmv2_hash[16];
 unsigned char *tiblob = NULL; /* target info blob */

 @@ -660,13 +667,14 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct 
 nls_table *nls_cp)
 }
 ses-auth_key.len += baselen;

 -   buf = (struct ntlmv2_resp *)
 +   ntlmv2 = (struct ntlmv2_resp *)
 (ses-auth_key.response + CIFS_SESS_KEY_SIZE);
 -   buf-blob_signature = cpu_to_le32(0x0101);
 -   buf-reserved = 0

Re: [PATCH] [RFC] vfs: don't revalidate dentries that serve as mountpoints

2013-11-04 Thread Shirish Pargaonkar

For a similar issue in cifs vfs (samba bugzilla 8950), I was going to try
unset the bit DCACHE_OP_REVALIDATE of d_flags of the
dentry.  Would something like work for the mountpoint dentry?



On Mon, Nov 4, 2013 at 7:16 PM, Jeff Layton  wrote:
> We had a couple of reports of people that are mounting NFS filesystems,
> and then bind mounting certain local files onto dentries in that nfs
> mount (sort of like a poor-man's unionmount).
>
> This all works well until the dentry serving as the mountpoint fails
> d_revalidate. The dentry will end up being invalidated which makes the
> bind mount unreachable via pathwalk.
>
> It doesn't make much sense to me to allow dentries to serve as
> mountpoints to end up invalidated, so there's no real point in
> attempting to d_revalidate them at all.
>
> Reported-by: Patrick McLean 
> Reported-by: Guang Cheng Li 
> Signed-off-by: Jeff Layton 
> ---
>  fs/namei.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index caa2805..5b10ad0 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -585,6 +585,9 @@ drop_root_mnt:
>
>  static inline int d_revalidate(struct dentry *dentry, unsigned int flags)
>  {
> +   /* dentries that serve as mountpoints are always considered valid */
> +   if (d_mountpoint(dentry))
> +   return 1;
> return dentry->d_op->d_revalidate(dentry, flags);
>  }
>
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] [RFC] vfs: don't revalidate dentries that serve as mountpoints

2013-11-04 Thread Shirish Pargaonkar

For a similar issue in cifs vfs (samba bugzilla 8950), I was going to try
unset the bit DCACHE_OP_REVALIDATE of d_flags of the
dentry.  Would something like work for the mountpoint dentry?



On Mon, Nov 4, 2013 at 7:16 PM, Jeff Layton jlay...@redhat.com wrote:
 We had a couple of reports of people that are mounting NFS filesystems,
 and then bind mounting certain local files onto dentries in that nfs
 mount (sort of like a poor-man's unionmount).

 This all works well until the dentry serving as the mountpoint fails
 d_revalidate. The dentry will end up being invalidated which makes the
 bind mount unreachable via pathwalk.

 It doesn't make much sense to me to allow dentries to serve as
 mountpoints to end up invalidated, so there's no real point in
 attempting to d_revalidate them at all.

 Reported-by: Patrick McLean chutz...@gentoo.org
 Reported-by: Guang Cheng Li ligua...@cn.ibm.com
 Signed-off-by: Jeff Layton jlay...@redhat.com
 ---
  fs/namei.c | 3 +++
  1 file changed, 3 insertions(+)

 diff --git a/fs/namei.c b/fs/namei.c
 index caa2805..5b10ad0 100644
 --- a/fs/namei.c
 +++ b/fs/namei.c
 @@ -585,6 +585,9 @@ drop_root_mnt:

  static inline int d_revalidate(struct dentry *dentry, unsigned int flags)
  {
 +   /* dentries that serve as mountpoints are always considered valid */
 +   if (d_mountpoint(dentry))
 +   return 1;
 return dentry-d_op-d_revalidate(dentry, flags);
  }

 --
 1.8.3.1

 --
 To unsubscribe from this list: send the line unsubscribe linux-nfs in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next] cifs: Cleanup and clarify CalcNTLMv2_response()

2013-10-19 Thread Shirish Pargaonkar

Tim,

Since when a NTLMv2 response is sent, contains a
session key, hmac-md5 digest, and the blob,
the offset has  + 8 to include 16 bytes of hmac-md5 digest.
hmac-md5 digest is based on 8 bytes of server challenge
and the blob.
So hmac-md5 digest (output) overwrites total 16 bytes in overall
ntlmv2 response after session key and is followed by the blob.

At this point in time, we are dealing with the opaque data, so casting those
16 bytes (after session key and before the blob) as ntlmv2_response
structure does not appear correct and can be confusing.

Regards,

Shirish

On Fri, Oct 18, 2013 at 11:51 AM, Tim Gardner  wrote:
> Use of a structure aids in the understanding of this
> seemingly simple bit of code. The addition of a couple
> of comments also helps.
>
> Cc: Jeff Layton 
> Cc: Steve French 
> Signed-off-by: Tim Gardner 
> ---
>
> I'd just like to be sure that the destructive copy is
> really what was intended. I can't tell from reading the MSDN
> section 3.3.2 NTLM v2 Authentication. It sort of makes
> my head hurt.
>
> rtg
>
>  fs/cifs/cifsencrypt.c |   21 -
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> index fc6f4f3..fecbfd0 100644
> --- a/fs/cifs/cifsencrypt.c
> +++ b/fs/cifs/cifsencrypt.c
> @@ -548,7 +548,9 @@ static int
>  CalcNTLMv2_response(const struct cifs_ses *ses, char *ntlmv2_hash)
>  {
> int rc;
> -   unsigned int offset = CIFS_SESS_KEY_SIZE + 8;
> +   struct ntlmv2_resp *resp = (struct ntlmv2_resp *)
> +   (ses->auth_key.response + CIFS_SESS_KEY_SIZE);
> +   unsigned int hashed_len = ses->auth_key.len - (CIFS_SESS_KEY_SIZE + 
> 8);
>
> if (!ses->server->secmech.sdeschmacmd5) {
> cifs_dbg(VFS, "%s: can't generate ntlmv2 hash\n", __func__);
> @@ -569,21 +571,30 @@ CalcNTLMv2_response(const struct cifs_ses *ses, char 
> *ntlmv2_hash)
> return rc;
> }
>
> +   /*
> +* The cryptkey is part of the buffer that feeds the MD5 hash, but
> +* gets over written later by the digest. See crypto_shash_final()
> +* below.
> +*/
> if (ses->server->negflavor == CIFS_NEGFLAVOR_EXTENDED)
> -   memcpy(ses->auth_key.response + offset,
> +   memcpy(>ntlmv2_hash[CIFS_SERVER_CHALLENGE_SIZE],
> ses->ntlmssp->cryptkey, CIFS_SERVER_CHALLENGE_SIZE);
> else
> -   memcpy(ses->auth_key.response + offset,
> +   memcpy(>ntlmv2_hash[CIFS_SERVER_CHALLENGE_SIZE],
> ses->server->cryptkey, CIFS_SERVER_CHALLENGE_SIZE);
> rc = crypto_shash_update(>server->secmech.sdeschmacmd5->shash,
> -   ses->auth_key.response + offset, ses->auth_key.len - offset);
> +   >ntlmv2_hash[8], hashed_len);
> if (rc) {
> cifs_dbg(VFS, "%s: Could not update with response\n", 
> __func__);
> return rc;
> }
>
> +   /*
> +* Yes - this is destructive. The 16 byte MD5 digest clobbers the
> +* cryptkey that was just copied into >ntlmv2_hash[8].
> +*/
> rc = crypto_shash_final(>server->secmech.sdeschmacmd5->shash,
> -   ses->auth_key.response + CIFS_SESS_KEY_SIZE);
> +   resp->ntlmv2_hash);
> if (rc)
> cifs_dbg(VFS, "%s: Could not generate md5 hash\n", __func__);
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH linux-next] cifs: Cleanup and clarify CalcNTLMv2_response()

2013-10-19 Thread Shirish Pargaonkar

Tim,

Since when a NTLMv2 response is sent, contains a
session key, hmac-md5 digest, and the blob,
the offset has  + 8 to include 16 bytes of hmac-md5 digest.
hmac-md5 digest is based on 8 bytes of server challenge
and the blob.
So hmac-md5 digest (output) overwrites total 16 bytes in overall
ntlmv2 response after session key and is followed by the blob.

At this point in time, we are dealing with the opaque data, so casting those
16 bytes (after session key and before the blob) as ntlmv2_response
structure does not appear correct and can be confusing.

Regards,

Shirish

On Fri, Oct 18, 2013 at 11:51 AM, Tim Gardner t...@tpi.com wrote:
 Use of a structure aids in the understanding of this
 seemingly simple bit of code. The addition of a couple
 of comments also helps.

 Cc: Jeff Layton jlay...@redhat.com
 Cc: Steve French sfre...@samba.org
 Signed-off-by: Tim Gardner t...@tpi.com
 ---

 I'd just like to be sure that the destructive copy is
 really what was intended. I can't tell from reading the MSDN
 section 3.3.2 NTLM v2 Authentication. It sort of makes
 my head hurt.

 rtg

  fs/cifs/cifsencrypt.c |   21 -
  1 file changed, 16 insertions(+), 5 deletions(-)

 diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
 index fc6f4f3..fecbfd0 100644
 --- a/fs/cifs/cifsencrypt.c
 +++ b/fs/cifs/cifsencrypt.c
 @@ -548,7 +548,9 @@ static int
  CalcNTLMv2_response(const struct cifs_ses *ses, char *ntlmv2_hash)
  {
 int rc;
 -   unsigned int offset = CIFS_SESS_KEY_SIZE + 8;
 +   struct ntlmv2_resp *resp = (struct ntlmv2_resp *)
 +   (ses-auth_key.response + CIFS_SESS_KEY_SIZE);
 +   unsigned int hashed_len = ses-auth_key.len - (CIFS_SESS_KEY_SIZE + 
 8);

 if (!ses-server-secmech.sdeschmacmd5) {
 cifs_dbg(VFS, %s: can't generate ntlmv2 hash\n, __func__);
 @@ -569,21 +571,30 @@ CalcNTLMv2_response(const struct cifs_ses *ses, char 
 *ntlmv2_hash)
 return rc;
 }

 +   /*
 +* The cryptkey is part of the buffer that feeds the MD5 hash, but
 +* gets over written later by the digest. See crypto_shash_final()
 +* below.
 +*/
 if (ses-server-negflavor == CIFS_NEGFLAVOR_EXTENDED)
 -   memcpy(ses-auth_key.response + offset,
 +   memcpy(resp-ntlmv2_hash[CIFS_SERVER_CHALLENGE_SIZE],
 ses-ntlmssp-cryptkey, CIFS_SERVER_CHALLENGE_SIZE);
 else
 -   memcpy(ses-auth_key.response + offset,
 +   memcpy(resp-ntlmv2_hash[CIFS_SERVER_CHALLENGE_SIZE],
 ses-server-cryptkey, CIFS_SERVER_CHALLENGE_SIZE);
 rc = crypto_shash_update(ses-server-secmech.sdeschmacmd5-shash,
 -   ses-auth_key.response + offset, ses-auth_key.len - offset);
 +   resp-ntlmv2_hash[8], hashed_len);
 if (rc) {
 cifs_dbg(VFS, %s: Could not update with response\n, 
 __func__);
 return rc;
 }

 +   /*
 +* Yes - this is destructive. The 16 byte MD5 digest clobbers the
 +* cryptkey that was just copied into resp-ntlmv2_hash[8].
 +*/
 rc = crypto_shash_final(ses-server-secmech.sdeschmacmd5-shash,
 -   ses-auth_key.response + CIFS_SESS_KEY_SIZE);
 +   resp-ntlmv2_hash);
 if (rc)
 cifs_dbg(VFS, %s: Could not generate md5 hash\n, __func__);

 --
 1.7.9.5

 --
 To unsubscribe from this list: send the line unsubscribe linux-cifs in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: cp --reflink and target file open flags

2013-10-17 Thread Shirish Pargaonkar

I think it would not matter which ioctl is used if the target file is
not opened with flag O_RDWR.

Perhaps cifs.ko can make the decision to change O_WRONLY
to O_RDWR based on the flag and location of the source and
destination files.


On Thu, Oct 17, 2013 at 5:45 AM, David Disseldorp  wrote:
> On Wed, 16 Oct 2013 18:36:19 -0500
> Steve French  wrote:
>
>> cp --reflink opens the target file for O_WRONLY before invoking the
>> (BTRFS) ioctl for clone file, but for copy offload over the network
>> the SMB2 specification requires that the target file be open O_RDWR.
>>
>> I may be able to upgrade the target file handle on the fly by
>> reopening it in cifs.ko, and of course I can write an SMB2/SMB3
>> specific copy command, but it would be preferable to allow use of cp
>> --reflink since so many people are familiar with it.
>>
>> There is quite a bit of flexibility in server side copy offload  -
>> more than cp an offer, especially when using SMB3 or later dialects
>> (e.g. in number of chunks sent at one time, chunk size, attributes
>> copied, and even whether to use T10 style offload), but still it would
>> be nice to support "cp --reflink" over the network.  Any ideas on
>> this?
>>
>> After looking at copy.c in coreutils for cp - I couldn't think of any
>> trivial way to force cp to open the target RW.
>>
>> Ideas?
>
> You should be able to avoid this by using FSCTL_SRV_COPYCHUNK_WRITE
> instead of FSCTL_SRV_COPYCHUNK on the wire. The former doesn't require
> read access on the target, while the latter does. See [MS-SMB2] 2.2.31
> and smbtorture's copy_chunk_bad_access test.
>
> Samba only supported FSCTL_SRV_COPYCHUNK until now, as that's what
> Windows Server 2k12 uses for copies initiated by Explorer. I've just
> sent out the trivial patches adding FSCTL_SRV_COPYCHUNK_WRITE support.
>
> Cheers, David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: cp --reflink and target file open flags

2013-10-17 Thread Shirish Pargaonkar

I think it would not matter which ioctl is used if the target file is
not opened with flag O_RDWR.

Perhaps cifs.ko can make the decision to change O_WRONLY
to O_RDWR based on the flag and location of the source and
destination files.


On Thu, Oct 17, 2013 at 5:45 AM, David Disseldorp dd...@suse.de wrote:
 On Wed, 16 Oct 2013 18:36:19 -0500
 Steve French smfre...@gmail.com wrote:

 cp --reflink opens the target file for O_WRONLY before invoking the
 (BTRFS) ioctl for clone file, but for copy offload over the network
 the SMB2 specification requires that the target file be open O_RDWR.

 I may be able to upgrade the target file handle on the fly by
 reopening it in cifs.ko, and of course I can write an SMB2/SMB3
 specific copy command, but it would be preferable to allow use of cp
 --reflink since so many people are familiar with it.

 There is quite a bit of flexibility in server side copy offload  -
 more than cp an offer, especially when using SMB3 or later dialects
 (e.g. in number of chunks sent at one time, chunk size, attributes
 copied, and even whether to use T10 style offload), but still it would
 be nice to support cp --reflink over the network.  Any ideas on
 this?

 After looking at copy.c in coreutils for cp - I couldn't think of any
 trivial way to force cp to open the target RW.

 Ideas?

 You should be able to avoid this by using FSCTL_SRV_COPYCHUNK_WRITE
 instead of FSCTL_SRV_COPYCHUNK on the wire. The former doesn't require
 read access on the target, while the latter does. See [MS-SMB2] 2.2.31
 and smbtorture's copy_chunk_bad_access test.

 Samba only supported FSCTL_SRV_COPYCHUNK until now, as that's what
 Windows Server 2k12 uses for copies initiated by Explorer. I've just
 sent out the trivial patches adding FSCTL_SRV_COPYCHUNK_WRITE support.

 Cheers, David
 --
 To unsubscribe from this list: send the line unsubscribe linux-cifs in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2 linux-next] cifs: Make big endian multiplex ID sequences monotonic on the wire

2013-10-16 Thread Shirish Pargaonkar

On Wed, Oct 16, 2013 at 10:09 AM, Tim Gardner  wrote:
> The multiplex identifier (MID) in the SMB header is only
> ever used by the client, in conjunction with PID, to match responses
> from the server. As such, the endianess of the MID is not important.
> However, When tracing packet sequences on the wire, protocol analyzers
> such as wireshark display MID as little endian. It is much more informative
> for the on-the-wire MID sequences to match debug information emitted by the
> CIFS driver.  Therefore, one should write and read MID in the SMB header
> assuming it is always little endian.
>
> Observed from wireshark during the protocol negotiation
> and session setup:
>
> Multiplex ID: 256
> Multiplex ID: 256
> Multiplex ID: 512
> Multiplex ID: 512
> Multiplex ID: 768
> Multiplex ID: 768
>
> After this patch on-the-wire MID values begin at 1 and increase monotonically.
>
> Introduce get_next_mid64() for the internal consumers that use the full 64 bit
> multiplex identifier.
>
> Introduce the helpers get_mid() and compare_mid() to make the endian
> translation clear.
>
> Cc: Steve French 
> Signed-off-by: Tim Gardner 
> ---
>
> I'm looking at some of this code in excrutiating detail because I'm having 
> trouble
> with a backport of CIFS from 3.9.7 on an embedded 2.6.31 powerpc. Its failing 
> the RawNTLMSSP
> authentication and is almost certainly an endian issue. x86 on the same code 
> base works
> quite well. Am I making a rash assumption that CIFS in 3.9 stable worked on 
> big endian ?

Do you have this commit  fdf96a907c1fbb93c633e2b7ede3b8df26d6a4c0 ?
This might help if you do not have it in your code base.

>
>  fs/cifs/cifsglob.h  |   25 -
>  fs/cifs/misc.c  |9 +
>  fs/cifs/smb1ops.c   |4 ++--
>  fs/cifs/smb2transport.c |2 +-
>  fs/cifs/transport.c |2 +-
>  5 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 52b6f6c..535e324 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -620,11 +620,34 @@ set_credits(struct TCP_Server_Info *server, const int 
> val)
>  }
>
>  static inline __u64
> -get_next_mid(struct TCP_Server_Info *server)
> +get_next_mid64(struct TCP_Server_Info *server)
>  {
> return server->ops->get_next_mid(server);
>  }
>
> +static inline __u16
> +get_next_mid(struct TCP_Server_Info *server)
> +{
> +   __u16 mid = get_next_mid64(server);
> +   /*
> +* The value in the SMB header should be little endian for easy
> +* on-the-wire decoding.
> +*/
> +   return cpu_to_le16(mid);
> +}
> +
> +static inline __u16
> +get_mid(const struct smb_hdr *smb)
> +{
> +   return le16_to_cpu(smb->Mid);
> +}
> +
> +static inline bool
> +compare_mid(__u16 mid, const struct smb_hdr *smb)
> +{
> +   return mid == le16_to_cpu(smb->Mid);
> +}
> +
>  /*
>   * When the server supports very large reads and writes via POSIX extensions,
>   * we can allow up to 2^24-1, minus the size of a READ/WRITE_AND_X header, 
> not
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 298e31e..c851d41 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -295,7 +295,8 @@ check_smb_hdr(struct smb_hdr *smb)
> if (smb->Command == SMB_COM_LOCKING_ANDX)
> return 0;
>
> -   cifs_dbg(VFS, "Server sent request, not response. mid=%u\n", 
> smb->Mid);
> +   cifs_dbg(VFS, "Server sent request, not response. mid=%u\n",
> +le16_to_cpu(smb->Mid));
> return 1;
>  }
>
> @@ -358,11 +359,11 @@ checkSMB(char *buf, unsigned int total_read)
> return 0; /* bcc wrapped */
> }
> cifs_dbg(FYI, "Calculated size %u vs length %u mismatch for 
> mid=%u\n",
> -clc_len, 4 + rfclen, smb->Mid);
> +clc_len, 4 + rfclen, le16_to_cpu(smb->Mid));
>
> if (4 + rfclen < clc_len) {
> cifs_dbg(VFS, "RFC1001 size %u smaller than SMB for 
> mid=%u\n",
> -rfclen, smb->Mid);
> +rfclen, le16_to_cpu(smb->Mid));
> return -EIO;
> } else if (rfclen > clc_len + 512) {
> /*
> @@ -375,7 +376,7 @@ checkSMB(char *buf, unsigned int total_read)
>  * data to 512 bytes.
>  */
> cifs_dbg(VFS, "RFC1001 size %u more than 512 bytes 
> larger than SMB for mid=%u\n",
> -rfclen, smb->Mid);
> +rfclen, le16_to_cpu(smb->Mid));
> return -EIO;
> }
> }
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index 8233b17..5598af9 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -67,7 +67,7 @@ send_nt_cancel(struct

Re: [PATCH 2/2 linux-next] cifs: Make big endian multiplex ID sequences monotonic on the wire

2013-10-16 Thread Shirish Pargaonkar

On Wed, Oct 16, 2013 at 10:09 AM, Tim Gardner t...@tpi.com wrote:
 The multiplex identifier (MID) in the SMB header is only
 ever used by the client, in conjunction with PID, to match responses
 from the server. As such, the endianess of the MID is not important.
 However, When tracing packet sequences on the wire, protocol analyzers
 such as wireshark display MID as little endian. It is much more informative
 for the on-the-wire MID sequences to match debug information emitted by the
 CIFS driver.  Therefore, one should write and read MID in the SMB header
 assuming it is always little endian.

 Observed from wireshark during the protocol negotiation
 and session setup:

 Multiplex ID: 256
 Multiplex ID: 256
 Multiplex ID: 512
 Multiplex ID: 512
 Multiplex ID: 768
 Multiplex ID: 768

 After this patch on-the-wire MID values begin at 1 and increase monotonically.

 Introduce get_next_mid64() for the internal consumers that use the full 64 bit
 multiplex identifier.

 Introduce the helpers get_mid() and compare_mid() to make the endian
 translation clear.

 Cc: Steve French sfre...@samba.org
 Signed-off-by: Tim Gardner t...@tpi.com
 ---

 I'm looking at some of this code in excrutiating detail because I'm having 
 trouble
 with a backport of CIFS from 3.9.7 on an embedded 2.6.31 powerpc. Its failing 
 the RawNTLMSSP
 authentication and is almost certainly an endian issue. x86 on the same code 
 base works
 quite well. Am I making a rash assumption that CIFS in 3.9 stable worked on 
 big endian ?

Do you have this commit  fdf96a907c1fbb93c633e2b7ede3b8df26d6a4c0 ?
This might help if you do not have it in your code base.


  fs/cifs/cifsglob.h  |   25 -
  fs/cifs/misc.c  |9 +
  fs/cifs/smb1ops.c   |4 ++--
  fs/cifs/smb2transport.c |2 +-
  fs/cifs/transport.c |2 +-
  5 files changed, 33 insertions(+), 9 deletions(-)

 diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
 index 52b6f6c..535e324 100644
 --- a/fs/cifs/cifsglob.h
 +++ b/fs/cifs/cifsglob.h
 @@ -620,11 +620,34 @@ set_credits(struct TCP_Server_Info *server, const int 
 val)
  }

  static inline __u64
 -get_next_mid(struct TCP_Server_Info *server)
 +get_next_mid64(struct TCP_Server_Info *server)
  {
 return server-ops-get_next_mid(server);
  }

 +static inline __u16
 +get_next_mid(struct TCP_Server_Info *server)
 +{
 +   __u16 mid = get_next_mid64(server);
 +   /*
 +* The value in the SMB header should be little endian for easy
 +* on-the-wire decoding.
 +*/
 +   return cpu_to_le16(mid);
 +}
 +
 +static inline __u16
 +get_mid(const struct smb_hdr *smb)
 +{
 +   return le16_to_cpu(smb-Mid);
 +}
 +
 +static inline bool
 +compare_mid(__u16 mid, const struct smb_hdr *smb)
 +{
 +   return mid == le16_to_cpu(smb-Mid);
 +}
 +
  /*
   * When the server supports very large reads and writes via POSIX extensions,
   * we can allow up to 2^24-1, minus the size of a READ/WRITE_AND_X header, 
 not
 diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
 index 298e31e..c851d41 100644
 --- a/fs/cifs/misc.c
 +++ b/fs/cifs/misc.c
 @@ -295,7 +295,8 @@ check_smb_hdr(struct smb_hdr *smb)
 if (smb-Command == SMB_COM_LOCKING_ANDX)
 return 0;

 -   cifs_dbg(VFS, Server sent request, not response. mid=%u\n, 
 smb-Mid);
 +   cifs_dbg(VFS, Server sent request, not response. mid=%u\n,
 +le16_to_cpu(smb-Mid));
 return 1;
  }

 @@ -358,11 +359,11 @@ checkSMB(char *buf, unsigned int total_read)
 return 0; /* bcc wrapped */
 }
 cifs_dbg(FYI, Calculated size %u vs length %u mismatch for 
 mid=%u\n,
 -clc_len, 4 + rfclen, smb-Mid);
 +clc_len, 4 + rfclen, le16_to_cpu(smb-Mid));

 if (4 + rfclen  clc_len) {
 cifs_dbg(VFS, RFC1001 size %u smaller than SMB for 
 mid=%u\n,
 -rfclen, smb-Mid);
 +rfclen, le16_to_cpu(smb-Mid));
 return -EIO;
 } else if (rfclen  clc_len + 512) {
 /*
 @@ -375,7 +376,7 @@ checkSMB(char *buf, unsigned int total_read)
  * data to 512 bytes.
  */
 cifs_dbg(VFS, RFC1001 size %u more than 512 bytes 
 larger than SMB for mid=%u\n,
 -rfclen, smb-Mid);
 +rfclen, le16_to_cpu(smb-Mid));
 return -EIO;
 }
 }
 diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
 index 8233b17..5598af9 100644
 --- a/fs/cifs/smb1ops.c
 +++ b/fs/cifs/smb1ops.c
 @@ -67,7 +67,7 @@ send_nt_cancel(struct TCP_Server_Info *server, void *buf,
 mutex_unlock(server-srv_mutex);

 cifs_dbg(FYI, issued NT_CANCEL for mid %u,

Re: Mount failure due to restricted access to a point along the mount path

2013-10-06 Thread Shirish Pargaonkar

So instead of breaking superblock sharing and fscache functionality
with 2), it may be better off to explore 1).  Will spend some time doing so.

Regards,

Shirish

On Fri, May 10, 2013 at 9:27 AM, Jeff Layton  wrote:
> On Fri, 10 May 2013 16:13:30 +0200
> Miklos Szeredi  wrote:
>
>> Hi,
>>
>> A while ago this was discussed:
>>
>>   http://thread.gmane.org/gmane.linux.kernel.cifs/7779
>>
>> This is essentially a regression introduced by the shared superblock
>> changes in 3.0 and several SUSE customers are complaining about it.
>> I've created a temporary fix which reverts 29 commits related to the
>> shared superblock changes.  It works, but it's obviously not a
>> permanent fix, especially since we definitely don't want to diverge
>> from mainline.
>>
>> Is this issue being worked on?  Don't other distros have similar reports?
>>
>> Thanks,
>> Miklos
>
> I don't know of anyone currently working on it. There are a couple of
> possible approaches to fixing it, I think:
>
> 1) if the dentries to get down to the root of the mount don't already
> exist, then attach some sort of "placeholder" inode that can be fleshed
> out later if and when the dentry is accessed via other means.
>
> 2) do something like what NFS does (see commit 54ceac45). This becomes
> a bit more complicated due to the fact that the server may not hand out
> real inode numbers and we sometimes have to fake them up.
>
> #1 is probably simpler to implement, but I'll confess that I haven't
> thought through all of the potential problems with it.
>
> --
> Jeff Layton 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Mount failure due to restricted access to a point along the mount path

2013-10-06 Thread Shirish Pargaonkar

So instead of breaking superblock sharing and fscache functionality
with 2), it may be better off to explore 1).  Will spend some time doing so.

Regards,

Shirish

On Fri, May 10, 2013 at 9:27 AM, Jeff Layton jlay...@redhat.com wrote:
 On Fri, 10 May 2013 16:13:30 +0200
 Miklos Szeredi mik...@szeredi.hu wrote:

 Hi,

 A while ago this was discussed:

   http://thread.gmane.org/gmane.linux.kernel.cifs/7779

 This is essentially a regression introduced by the shared superblock
 changes in 3.0 and several SUSE customers are complaining about it.
 I've created a temporary fix which reverts 29 commits related to the
 shared superblock changes.  It works, but it's obviously not a
 permanent fix, especially since we definitely don't want to diverge
 from mainline.

 Is this issue being worked on?  Don't other distros have similar reports?

 Thanks,
 Miklos

 I don't know of anyone currently working on it. There are a couple of
 possible approaches to fixing it, I think:

 1) if the dentries to get down to the root of the mount don't already
 exist, then attach some sort of placeholder inode that can be fleshed
 out later if and when the dentry is accessed via other means.

 2) do something like what NFS does (see commit 54ceac45). This becomes
 a bit more complicated due to the fact that the server may not hand out
 real inode numbers and we sometimes have to fake them up.

 #1 is probably simpler to implement, but I'll confess that I haven't
 thought through all of the potential problems with it.

 --
 Jeff Layton jlay...@redhat.com
 --
 To unsubscribe from this list: send the line unsubscribe linux-cifs in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

NIST SP800-138 availibility using kernel crypto APIs for SMB3.0 MAC generation

2013-05-19 Thread Shirish Pargaonkar

With the recent patches added to kernel crypto for improving AES
support, adding aesni etc, it seems like it is time to add AES CMAC
to the cifs kernel module (for the popular SMB3 signing and per-share
encryption) but needed for an implementation for SP800-138 in kernel
crypto codebase.
Was specifically interested using the way defined in 3.1.4.2 of
MS-SMB2 document, in particular

KDF in Counter mode (section 5.1 of NIST SP 800-108) or a way to specify a mode
PRF as HMAC-SHA256 (or a way to specify a PRF)


cifs client would use this algorithm for SMB3.0 MAC generation
(network packet signing on network file system mounts to newer
generation NAS, and Windows 2012) and also for per-share encryption
(which is also available on the most recent generation of NAS via
SMB3). Apparently with current Intel processors having hardware for
this kind of encryption offload - full packet encryption is faster
than packet signing used to be (with the older standard algorithms)
and just doing packet signing is really fast.

Are their APIs in crypto kernel code to use for this purpose?

Regards,

Shirish
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

NIST SP800-138 availibility using kernel crypto APIs for SMB3.0 MAC generation

2013-05-19 Thread Shirish Pargaonkar

With the recent patches added to kernel crypto for improving AES
support, adding aesni etc, it seems like it is time to add AES CMAC
to the cifs kernel module (for the popular SMB3 signing and per-share
encryption) but needed for an implementation for SP800-138 in kernel
crypto codebase.
Was specifically interested using the way defined in 3.1.4.2 of
MS-SMB2 document, in particular

KDF in Counter mode (section 5.1 of NIST SP 800-108) or a way to specify a mode
PRF as HMAC-SHA256 (or a way to specify a PRF)


cifs client would use this algorithm for SMB3.0 MAC generation
(network packet signing on network file system mounts to newer
generation NAS, and Windows 2012) and also for per-share encryption
(which is also available on the most recent generation of NAS via
SMB3). Apparently with current Intel processors having hardware for
this kind of encryption offload - full packet encryption is faster
than packet signing used to be (with the older standard algorithms)
and just doing packet signing is really fast.

Are their APIs in crypto kernel code to use for this purpose?

Regards,

Shirish
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

50 matches

Mail list logo