Hi Stefan, Good points. Please read my replies inline below. On Sun, Nov 7, 2021 at 4:19 PM Stefan Metzmacher <metze@xxxxxxxxx> wrote: > > > Hi Shyam, > > >>>> 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 think that's okay. AFAIK, that's only used for > > debugging/troubleshooting purposes. So it doesn't need to be a 100% > > accurate. > > That's not true, the workstation name is used for access checks. > > [MS-ADA3] 2.353 Attribute userWorkstations > https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-ada3/ec941bac-bc77-48f3-a1cf-79d773a91b6b > I see. So ideally we should be setting the FQDN of the client here. I don't know if there's a way to get the FQDN from within the kernel. I'll just set the utsname()->nodename for now. If someone has better ideas, feel free to chime in. > >>> > >>> - 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. > > I initially did not have the utsname changes. That idea was an afterthought. > > Sure. I'll update the patch to fix this. > > > >>> > >>> - 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) > > Noted. > > > >>> > >>> - 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. > > That's a good idea. Let me explore that too. > > No, it's not this is for windows version numbers. > https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-nlmp/a211d894-21bc-4b8b-86ba-b83d0c167b00#Appendix_A_32 > > If you want to encode something you can use > 2.2.2.2 Single_Host_Data > > metze Thanks for the tip. I'll explore this option for a future fix. -- Regards, Shyam
