2022-10-03 6:21 GMT+09:00, Atte Heikkilä <atteh.mailbox@xxxxxxxxx>:
> On Sun, 2 Oct 2022 14:34:43 -0400, Tom Talpey wrote:
>> On 10/2/2022 11:40 AM, Atte Heikkilä wrote:
>>> On Sun, 2 Oct 2022 05:46:28 +0300, Atte Heikkilä wrote:
>>>> ...
>>>> diff --git a/fs/ksmbd/ksmbd_netlink.h b/fs/ksmbd/ksmbd_netlink.h
>>>> index e0cbcfa98c7e..ff07c67f4565 100644
>>>> --- a/fs/ksmbd/ksmbd_netlink.h
>>>> +++ b/fs/ksmbd/ksmbd_netlink.h
>>>> @@ -163,7 +163,8 @@ struct ksmbd_share_config_response {
>>>> __u16 force_directory_mode;
>>>> __u16 force_uid;
>>>> __u16 force_gid;
>>>> - __u32 reserved[128]; /* Reserved room */
>>>> + __s8 share_name[KSMBD_REQ_MAX_SHARE_NAME];
>>>> + __u32 reserved[112]; /* Reserved room */
>>
>> I notice you still have "112" here, did you reject my suggestion
>> to code the size relative to KSMBD_REQ_MAX_SHARE_NAME?
>>
>
> If size of `reserved' should be relative, then it would be:
> 128 - DIV_ROUND_UP(KSMBD_REQ_MAX_SHARE_NAME, sizeof(__u32))
>
> Since ksmbd-tools already has the 112 (128-64/4), I thought to keep the
> kernel side consistent with it for now.
>
>> Either way I think I made a flawed suggestion. The "reserved" field
>> is __u32 but the KSMBD_REQ_MAX_SHARE_NAME is __s8. So, two things:
>>
>> - why is share_name an __s8? Wouldn't __u8 be more appropriate?
>
> As I understand it, the only reason for `__s8' over `__u8' here is that
> `char' is most often a signed type.
>
>> - why is reserved a __u32? ISTM that __u8 would also be a better
>> choice, and then the size would be "512 - KSMBD_REQ_MAX_SHARE_NAME".
>>
>>
>
> I don't know why `reserved' is `__u32'. I agree that it would be better
> if it was `__u8'.
We can make another patch for both ksmbd/ksmbd-tools later.
>
>>>> __u32 veto_list_sz;
>>>> __s8 ____payload[];
>>>> };
>>>> diff --git a/fs/ksmbd/mgmt/share_config.c
>>>> b/fs/ksmbd/mgmt/share_config.c
>>>> index 5d039704c23c..dfb4bb365891 100644
>>>> --- a/fs/ksmbd/mgmt/share_config.c
>>>> +++ b/fs/ksmbd/mgmt/share_config.c
>>>> @@ -16,6 +16,7 @@
>>>> #include "user_config.h"
>>>> #include "user_session.h"
>>>> #include "../transport_ipc.h"
>>>> +#include "../misc.h"
>>>>
>>>> #define SHARE_HASH_BITS 3
>>>> static DEFINE_HASHTABLE(shares_table, SHARE_HASH_BITS);
>>>> @@ -119,7 +120,8 @@ static int parse_veto_list(struct ksmbd_share_config
>>>> *share,
>>>> return 0;
>>>> }
>>>>
>>>> -static struct ksmbd_share_config *share_config_request(const char
>>>> *name)
>>>> +static struct ksmbd_share_config *share_config_request(struct
>>>> unicode_map *um,
>>>> + const char *name)
>>>> {
>>>> struct ksmbd_share_config_response *resp;
>>>> struct ksmbd_share_config *share = NULL;
>>>> @@ -133,6 +135,17 @@ static struct ksmbd_share_config
>>>> *share_config_request(const char *name)
>>>> if (resp->flags == KSMBD_SHARE_FLAG_INVALID)
>>>> goto out;
>>>>
>>>> + if (*resp->share_name) {
>>>> + char *cf_resp_name;
>>>> + bool equal;
>>>> +
>>>> + cf_resp_name = ksmbd_casefold_sharename(um, resp->share_name);
>>>> + equal = !IS_ERR(cf_resp_name) && !strcmp(cf_resp_name, name);
>>>> + kfree(cf_resp_name);
>>>
>>> Well, kfree() is *not* a no-op for ERR_PTR() like it is for NULL so
>>> this patch is not good either. At least I'm running out of ways to get
>>> this wrong.
>>
>> :)
>>
>> Tom.
>>
>>
>>>
>>>> + if (!equal)
>>>> + goto out;
>>>> + }
>>>> +
>>>> share = kzalloc(sizeof(struct ksmbd_share_config), GFP_KERNEL);
>>>> if (!share)
>>>> goto out;
>>>> @@ -190,7 +203,8 @@ static struct ksmbd_share_config
>>>> *share_config_request(const char *name)
>>>> return share;
>>>> }
>>>>
>>>> -struct ksmbd_share_config *ksmbd_share_config_get(const char *name)
>>>> +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map
>>>> *um,
>>>> + const char *name)
>>>> {
>>>> struct ksmbd_share_config *share;
>>>>
>>>> @@ -202,7 +216,7 @@ struct ksmbd_share_config
>>>> *ksmbd_share_config_get(const char *name)
>>>>
>>>> if (share)
>>>> return share;
>>>> - return share_config_request(name);
>>>> + return share_config_request(um, name);
>>>> }
>>>>
>>>> bool ksmbd_share_veto_filename(struct ksmbd_share_config *share,
>>>> diff --git a/fs/ksmbd/mgmt/share_config.h
>>>> b/fs/ksmbd/mgmt/share_config.h
>>>> index 7f7e89ecfe61..3fd338293942 100644
>>>> --- a/fs/ksmbd/mgmt/share_config.h
>>>> +++ b/fs/ksmbd/mgmt/share_config.h
>>>> @@ -9,6 +9,7 @@
>>>> #include <linux/workqueue.h>
>>>> #include <linux/hashtable.h>
>>>> #include <linux/path.h>
>>>> +#include <linux/unicode.h>
>>>>
>>>> struct ksmbd_share_config {
>>>> char *name;
>>>> @@ -74,7 +75,8 @@ static inline void ksmbd_share_config_put(struct
>>>> ksmbd_share_config *share)
>>>> __ksmbd_share_config_put(share);
>>>> }
>>>>
>>>> -struct ksmbd_share_config *ksmbd_share_config_get(const char *name);
>>>> +struct ksmbd_share_config *ksmbd_share_config_get(struct unicode_map
>>>> *um,
>>>> + const char *name);
>>>> bool ksmbd_share_veto_filename(struct ksmbd_share_config *share,
>>>> const char *filename);
>>>> #endif /* __SHARE_CONFIG_MANAGEMENT_H__ */
>>>> diff --git a/fs/ksmbd/mgmt/tree_connect.c
>>>> b/fs/ksmbd/mgmt/tree_connect.c
>>>> index 867c0286b901..8ce17b3fb8da 100644
>>>> --- a/fs/ksmbd/mgmt/tree_connect.c
>>>> +++ b/fs/ksmbd/mgmt/tree_connect.c
>>>> @@ -26,7 +26,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn,
>>>> struct ksmbd_session *sess,
>>>> struct sockaddr *peer_addr;
>>>> int ret;
>>>>
>>>> - sc = ksmbd_share_config_get(share_name);
>>>> + sc = ksmbd_share_config_get(conn->um, share_name);
>>>> if (!sc)
>>>> return status;
>>>>
>>>> @@ -61,7 +61,7 @@ ksmbd_tree_conn_connect(struct ksmbd_conn *conn,
>>>> struct ksmbd_session *sess,
>>>> struct ksmbd_share_config *new_sc;
>>>>
>>>> ksmbd_share_config_del(sc);
>>>> - new_sc = ksmbd_share_config_get(share_name);
>>>> + new_sc = ksmbd_share_config_get(conn->um, share_name);
>>>> if (!new_sc) {
>>>> pr_err("Failed to update stale share config\n");
>>>> status.ret = -ESTALE;
>>>> diff --git a/fs/ksmbd/misc.c b/fs/ksmbd/misc.c
>>>> index 28459b1efaa8..9e8afaa686e3 100644
>>>> --- a/fs/ksmbd/misc.c
>>>> +++ b/fs/ksmbd/misc.c
>>>> @@ -227,7 +227,7 @@ void ksmbd_conv_path_to_windows(char *path)
>>>> strreplace(path, '/', '\\');
>>>> }
>>>>
>>>> -static char *casefold_sharename(struct unicode_map *um, const char
>>>> *name)
>>>> +char *ksmbd_casefold_sharename(struct unicode_map *um, const char
>>>> *name)
>>>> {
>>>> char *cf_name;
>>>> int cf_len;
>>>> @@ -273,7 +273,7 @@ char *ksmbd_extract_sharename(struct unicode_map
>>>> *um, const char *treename)
>>>> name = (pos + 1);
>>>>
>>>> /* caller has to free the memory */
>>>> - return casefold_sharename(um, name);
>>>> + return ksmbd_casefold_sharename(um, name);
>>>> }
>>>>
>>>> /**
>>>> diff --git a/fs/ksmbd/misc.h b/fs/ksmbd/misc.h
>>>> index cc72f4e6baf2..1facfcd21200 100644
>>>> --- a/fs/ksmbd/misc.h
>>>> +++ b/fs/ksmbd/misc.h
>>>> @@ -20,6 +20,7 @@ int get_nlink(struct kstat *st);
>>>> void ksmbd_conv_path_to_unix(char *path);
>>>> void ksmbd_strip_last_slash(char *path);
>>>> void ksmbd_conv_path_to_windows(char *path);
>>>> +char *ksmbd_casefold_sharename(struct unicode_map *um, const char
>>>> *name);
>>>> char *ksmbd_extract_sharename(struct unicode_map *um, const char
>>>> *treename);
>>>> char *convert_to_unix_name(struct ksmbd_share_config *share, const
>>>> char *name);
>>>>
>>>> --
>>>> 2.37.3
>>>>
>>>>
>>>
>>
>
