| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 20 Aug 2013 16:20:07 +0800
From: Chen Gang <gang.chen@...anux.com>
To: Chen Gang F T <chen.gang.flying.transformer@...il.com>
CC: Cyrill Gorcunov <gorcunov@...il.com>, Mel Gorman <mgorman@...e.de>,
kosaki.motohiro@...fujitsu.com, riel@...hat.com, hughd@...gle.com,
xemul@...allels.com, rientjes@...gle.com,
Wanpeng Li <liwanp@...ux.vnet.ibm.com>,
Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
Sorry for reply multiple times to bother many members.
It seems, I am tired, and need have a rest today. :-(
On 08/20/2013 04:13 PM, Chen Gang F T wrote:
> On 08/20/2013 04:09 PM, Chen Gang wrote:
>> On 08/20/2013 03:51 PM, Chen Gang wrote:
>>> On 08/20/2013 03:48 PM, Chen Gang wrote:
>>>> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>>>>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>>>>
>>>>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>>>>> Won't this be more short and convenient?
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>>>>> (but need let outside know about the string truncation).
>>>>>>
>>>>>> So I still suggest to give more check for it.
>>>>>
>>>>> I still don't like adding additional code like
>>>>>
>>>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>>>> + if (ret < 0)
>>>>> + switch (ret) {
>>>>> + case -ENOSPC:
>>>>> + printk(KERN_WARNING
>>>>> + "in %s: string is truncated in mpol_to_str().\n",
>>>>> + __func__);
>>>
>>> Oh, that need 'break' in my original patch. :-)
>>>
>>>>> + default:
>>>>> + printk(KERN_ERR
>>>>> + "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>>>>> + __func__, ret, buffer, sizeof(buffer), mpol);
>>>>> + return;
>>>>> + }
>>>>>
>>>>> this code is pretty neat for debugging purpose I think but in most case (if
>>>>> only I've not missed something obvious) it simply won't be the case.
>>>>>
>>>>
>>>> For mpol_to_str(), it is for printing string, I suggest to fill buffer
>>>> as full as possible like another printing string functions, -ENOSPC is
>>>> not critical error, callers may can bear it, and still want to continue.
>>>>
>>>> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
>>>> really not a critical error, they can continue.
>>>>
>>>> For the 'default' error processing:
>>>>
>>>> I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
>>>> Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
>>>>
>>>>
>>>> Thanks.
>>>>
>>
>> Oh, for '-ENOSPC', it means critical error, it is my fault.
>>
>> So, for simplify thinking and implementation, use your patch below is OK
>> to me (but I suggest to print error information in the none return value
>> function).
>>
>> :-)
>>
>>>>> Won't somthing like below do the same but with smaller code change?
>>>>> Note I've not even compiled it but it shows the idea.
>>>>> ---
>>>>> fs/proc/task_mmu.c | 4 +++-
>>>>> mm/shmem.c | 17 +++++++++--------
>>>>> 2 files changed, 12 insertions(+), 9 deletions(-)
>>>>>
>>>>> Index: linux-2.6.git/fs/proc/task_mmu.c
>>>>> ===================================================================
>>>>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>>>>> +++ linux-2.6.git/fs/proc/task_mmu.c
>>>>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>>>>> walk.mm = mm;
>>>>>
>>>>> pol = get_vma_policy(task, vma, vma->vm_start);
>>>>> - mpol_to_str(buffer, sizeof(buffer), pol);
>>>>> + n = mpol_to_str(buffer, sizeof(buffer), pol);
>>>>> mpol_cond_put(pol);
>>>>> + if (n < 0)
>>>>> + return n;
>>>>>
>>>>> seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>>>>
>>>>> Index: linux-2.6.git/mm/shmem.c
>>>>> ===================================================================
>>>>> --- linux-2.6.git.orig/mm/shmem.c
>>>>> +++ linux-2.6.git/mm/shmem.c
>>>>> @@ -883,16 +883,20 @@ redirty:
>>>>>
>>>>> #ifdef CONFIG_NUMA
>>>>> #ifdef CONFIG_TMPFS
>>>>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>>> {
>>>>> char buffer[64];
>>>>> + int ret;
>>>>>
>>>>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>>>>> - return; /* show nothing */
>>>>> + return 0; /* show nothing */
>>>>>
>>>>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>>>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>>
>>>>> seq_printf(seq, ",mpol=%s", buffer);
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>>>>> @@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
>>>>> }
>>>>> #else /* !CONFIG_NUMA */
>>>>> #ifdef CONFIG_TMPFS
>>>>> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>>> -{
>>>>> -}
>>>>> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
>>>>> #endif /* CONFIG_TMPFS */
>>>>>
>>>>> static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>>>>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>>>>> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>>>>> seq_printf(seq, ",gid=%u",
>>>>> from_kgid_munged(&init_user_ns, sbinfo->gid));
>>>>> - shmem_show_mpol(seq, sbinfo->mpol);
>>>>> - return 0;
>>>>> + return shmem_show_mpol(seq, sbinfo->mpol);
>>>>> }
>>>>> #endif /* CONFIG_TMPFS */
>>>>>
>>>>>
>>>>>
>
> Oh, you have done, sorry.
>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
--
Chen Gang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists