All of lore.kernel.org
 help / color / mirror / Atom feed
From: Enzo Matsumiya <ematsumiya@suse.de>
To: Shyam Prasad N <nspmangalore@gmail.com>
Cc: Steve French <smfrench@gmail.com>,
	CIFS <linux-cifs@vger.kernel.org>, Paulo Alcantara <pc@cjr.nz>
Subject: Re: [PATCH] cifs: send workstation name during ntlmssp session setup
Date: Fri, 5 Nov 2021 22:38:54 -0300	[thread overview]
Message-ID: <20211106013854.6qx3tz53pvayqcgm@cyberdelia> (raw)
In-Reply-To: <CANT5p=rgHn59NVvH32FSKtNv_cyKi4ATSAExBmWT_qjb7km7Fw@mail.gmail.com>

Hi Shyam,

I have some observations/suggestions regarding your patch:

On 11/06, Shyam Prasad N wrote:
>Hi Steve,
>
>Please review this patch, and let me know what you think.
>Having this info in the workstation field of session setup helps
>server debugging in two ways.
>1. It helps identify the client by node name.
>2. It helps get the kernel release running on the client side.
>
>https://github.com/sprasad-microsoft/smb3-kernel-client/commit/d988e704dd9170c19ff94d9421c017e65dbbaac1.patch

- AFAICS it doesn't consider runtime hostname changes. Is it important
   to keep track of it? Would changing it mid-auth steps break it
   somehow?

- I didn't understand the purpose of CIFS_DEFAULT_WORKSTATION_NAME. Why
   not simply use utsname()->nodename? Or even init_utsname()->nodename, which
   is supposed to be always valid.

- Ditto for CIFS_MAX_WORKSTATION_LEN. utsname()->nodename has at most 65
   bytes (__NEW_UTS_LEN + 1) anyway. Perhaps using MAXHOSTNAMELEN from
   <asm/param.h> would be a more generic approach.
   (btw this is because nodename is the unqualified hostname, sans-domain)

- Instead of setting workstation_name to "nodename:release", why not
   implement the VERSION structure (MS-NLMP 2.2.2.10)? Then use
   LINUX_VERSION_* from <linux/version.h> or parse utsname()->release.

>I ran some basic testing with the patch. Seems to serve the purpose.
>Please let me know if I'm missing something.

I hope I didn't miss anything.


Cheers,

Enzo

  reply	other threads:[~2021-11-06  1:39 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-05 19:22 [PATCH] cifs: send workstation name during ntlmssp session setup Shyam Prasad N
2021-11-06  1:38 ` Enzo Matsumiya [this message]
     [not found]   ` <CAH2r5mvQG0DFmMdzojH2u_w2=_9oGRV++AnEt_d7WJzj=-uTKA@mail.gmail.com>
2021-11-06  3:38     ` Shyam Prasad N
2021-11-06  7:02       ` Shyam Prasad N
2021-11-07 10:49       ` Stefan Metzmacher
2021-11-08  7:11         ` Shyam Prasad N

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211106013854.6qx3tz53pvayqcgm@cyberdelia \
    --to=ematsumiya@suse.de \
    --cc=linux-cifs@vger.kernel.org \
    --cc=nspmangalore@gmail.com \
    --cc=pc@cjr.nz \
    --cc=smfrench@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.