Re: [PATCH] inotify: add error handling for kmem_cache_create
> 在 2018年6月12日，下午12:22，Zhouyang Jia 写道： > > When kmem_cache_create fails, the lack of error-handling code may > cause unexpected results. > > This patch adds error-handling code after calling kmem_cache_create. I think SLAB_PANIC can handle this case. Thanks, Chengguang. > > Signed-off-by: Zhouyang Jia > --- > fs/notify/inotify/inotify_user.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/notify/inotify/inotify_user.c > b/fs/notify/inotify/inotify_user.c > index ef32f36..0704bab 100644 > --- a/fs/notify/inotify/inotify_user.c > +++ b/fs/notify/inotify/inotify_user.c > @@ -805,6 +805,8 @@ static int __init inotify_user_setup(void) > BUG_ON(hweight32(ALL_INOTIFY_BITS) != 21); > > inotify_inode_mark_cachep = KMEM_CACHE(inotify_inode_mark, SLAB_PANIC); > + if (!inotify_inode_mark_cachep) > + return -ENOMEM; > > inotify_max_queued_events = 16384; > init_user_ns.ucount_max[UCOUNT_INOTIFY_INSTANCES] = 128; > -- > 2.7.4 >
Re: [RESEND PATCH] x86: remove redundant check for kmem_cache_create()
So still need to keep return type as int, I’ll fix in V2.
Thanks.
> 在 2018年6月12日，下午7:27，kbuild test robot 写道：
>
> Hi Chengguang,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on tip/x86/core]
> [also build test WARNING on v4.17 next-20180612]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Chengguang-Xu/x86-remove-redundant-check-for-kmem_cache_create/20180612-182134
> config: i386-randconfig-a1-201823 (attached as .config)
> compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
> reproduce:
># save the attached .config to linux build tree
>make ARCH=i386
>
> All warnings (new ones prefixed by >>):
>
> In file included from include/linux/printk.h:5:0,
>from include/linux/kernel.h:13,
>from include/asm-generic/bug.h:15,
>from arch/x86/include/asm/bug.h:81,
>from include/linux/bug.h:4,
>from include/linux/mmdebug.h:4,
>from include/linux/mm.h:8,
>from arch/x86/mm/pgtable.c:1:
>>> arch/x86/mm/pgtable.c:327:15: warning: initialization from incompatible
>>> pointer type
>core_initcall(pgd_cache_init);
> ^
> include/linux/init.h:166:58: note: in definition of macro
> '__define_initcall'
> __attribute__((__section__(".initcall" #id ".init"))) = fn;
> ^
> arch/x86/mm/pgtable.c:327:1: note: in expansion of macro 'core_initcall'
>core_initcall(pgd_cache_init);
>^
>
> vim +327 arch/x86/mm/pgtable.c
>
> 1db491f7 Fenghua Yu2015-01-15 308
> d524db50 Chengguang Xu 2018-06-12 309 static void __init
> pgd_cache_init(void)
> 1db491f7 Fenghua Yu2015-01-15 310 {
> 1db491f7 Fenghua Yu2015-01-15 311/*
> 1db491f7 Fenghua Yu2015-01-15 312 * When PAE kernel is running
> as a Xen domain, it does not use
> 1db491f7 Fenghua Yu2015-01-15 313 * shared kernel pmd. And this
> requires a whole page for pgd.
> 1db491f7 Fenghua Yu2015-01-15 314 */
> 1db491f7 Fenghua Yu2015-01-15 315if (!SHARED_KERNEL_PMD)
> d524db50 Chengguang Xu 2018-06-12 316return;
> 1db491f7 Fenghua Yu2015-01-15 317
> 1db491f7 Fenghua Yu2015-01-15 318/*
> 1db491f7 Fenghua Yu2015-01-15 319 * when PAE kernel is not
> running as a Xen domain, it uses
> 1db491f7 Fenghua Yu2015-01-15 320 * shared kernel pmd. Shared
> kernel pmd does not require a whole
> 1db491f7 Fenghua Yu2015-01-15 321 * page for pgd. We are able to
> just allocate a 32-byte for pgd.
> 1db491f7 Fenghua Yu2015-01-15 322 * During boot time, we create
> a 32-byte slab for pgd table allocation.
> 1db491f7 Fenghua Yu2015-01-15 323 */
> 1db491f7 Fenghua Yu2015-01-15 324pgd_cache =
> kmem_cache_create("pgd_cache", PGD_SIZE, PGD_ALIGN,
> 1db491f7 Fenghua Yu2015-01-15 325
> SLAB_PANIC, NULL);
> 1db491f7 Fenghua Yu2015-01-15 326 }
> 1db491f7 Fenghua Yu2015-01-15 @327 core_initcall(pgd_cache_init);
> 1db491f7 Fenghua Yu2015-01-15 328
>
> :: The code at line 327 was first introduced by commit
> :: 1db491f77b6ed0f32f1d4a3ac40a5be9524f1914 x86/mm: Reduce PAE-mode per
> task pgd allocation overhead from 4K to 32 bytes
>
> :: TO: Fenghua Yu
> :: CC: Ingo Molnar
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
> <.config.gz>
Re: [V9fs-developer] [PATCH v2 1/2] net/9p: detecting invalid options as much as possible
Thanks for your review, I’ll fix the issue in v3.
> 在 2018年5月3日，上午6:32，Andrew Morton 写道：
>
> On Tue, 17 Apr 2018 14:45:01 +0800 Chengguang Xu wrote:
>
>> Currently when detecting invalid options in option parsing,
>> some options(e.g. msize) just set errno and allow to continuously
>> validate other options so that it can detect invalid options
>> as much as possible and give proper error messages together.
>>
>> This patch applies same rule to option 'trans' and 'version'
>> when detecting EINVAL.
>>
>> ...
>
> A few issues:
>
>> --- a/net/9p/client.c
>> +++ b/net/9p/client.c
>> @@ -199,7 +199,7 @@ static int parse_opts(char *opts, struct p9_client *clnt)
>> s);
>> ret = -EINVAL;
>> kfree(s);
>> -goto free_and_return;
>> +continue;
>> }
>> kfree(s);
>> break;
>
> Here we can just do
>
> --- a/net/9p/client.c~net-9p-detecting-invalid-options-as-much-as-possible-fix
> +++ a/net/9p/client.c
> @@ -198,8 +198,6 @@ static int parse_opts(char *opts, struct
> pr_info("Could not find request transport:
> %s\n",
> s);
> ret = -EINVAL;
> - kfree(s);
> - continue;
> }
> kfree(s);
> break;
>
>> @@ -217,7 +217,7 @@ static int parse_opts(char *opts, struct p9_client *clnt)
>> ret = get_protocol_version(s);
>
> And here's the patch introduces a bug: if `ret' has value -EINVAL from
> an earlier parsing error, this code will overwrite that error code with
> zero.
>
> So you'll need to introduce a new temporary variable here. And I
> suggest that for future-safety, we permit all error returns from
> get_protocol_version(), not just -EINVAL. So something like:
>
> --- a/net/9p/client.c~net-9p-detecting-invalid-options-as-much-as-possible-fix
> +++ a/net/9p/client.c
> @@ -214,13 +214,12 @@ static int parse_opts(char *opts, struct
>"problem allocating copy of version
> arg\n");
> goto free_and_return;
> }
> - ret = get_protocol_version(s);
> - if (ret == -EINVAL) {
> - kfree(s);
> - continue;
> - }
> + pv = get_protocol_version(s);
> + if (pv >= 0)
> + clnt->proto_version = pv;
> + else
> + ret = pv;
> kfree(s);
> - clnt->proto_version = ret;
> break;
> default:
> continue;
> _
>
>
> Similar changes can be made to patch 2/2.
>
Re: [PATCH 1/2] hfs: fix potential refcnt problem of nls module
在 2018年4月19日，上午3:42，Andrew Morton 写道：
>
> On Tue, 17 Apr 2018 15:05:32 +0800 Chengguang Xu wrote:
>
>> When specifying iocharset/codepage multiple times in a mount,
>> current option parsing will cause inaccurate refcount of nls
>> module. Hence, call unload_nls for previous one in this case.
>>
>> ...
>>
>> --- a/fs/hfs/super.c
>> +++ b/fs/hfs/super.c
>> @@ -329,8 +329,10 @@ static int parse_options(char *options, struct
>> hfs_sb_info *hsb)
>> return 0;
>> }
>> p = match_strdup(&args[0]);
>> -if (p)
>> +if (p) {
>> +unload_nls(hsb->nls_disk);
>> hsb->nls_disk = load_nls(p);
>> +}
>> if (!hsb->nls_disk) {
>> pr_err("unable to load codepage \"%s\"\n", p);
>> kfree(p);
>> @@ -344,8 +346,10 @@ static int parse_options(char *options, struct
>> hfs_sb_info *hsb)
>> return 0;
>> }
>> p = match_strdup(&args[0]);
>> -if (p)
>> +if (p) {
>> +unload_nls(hsb->nls_io);
>> hsb->nls_io = load_nls(p);
>> +}
>> if (!hsb->nls_io) {
>> pr_err("unable to load iocharset \"%s\"\n", p);
>> kfree(p);
>
> Confused.
>
> break;
> : case opt_codepage:
> : if (hsb->nls_disk) {
> : pr_err("unable to change codepage\n");
> : return 0;
> : }
>
> Here, hsb->nls_disk is known to be zero.
>
> : p = match_strdup(&args[0]);
> : if (p) {
> : unload_nls(hsb->nls_disk);
>
> So this will always do unload_nls(0).
>
> : hsb->nls_disk = load_nls(p);
> : }
>
> And the same applies to your opt_iocharset change.
You are right. Sorry I just misread this part, please just drop the patch.
Thanks.
