Re: [PATCH V2 2/3] powerpc, ptrace: Enable support for transactional memory register sets

[Anshuman Khandual]

On 05/19/2014 08:13 PM, Pedro Alves wrote:
> On 05/19/2014 12:46 PM, Anshuman Khandual wrote:
> 
>>>> I couldn't actually find any arch that currently returns -ENODEV in
>>>> the "active" hook.  I see that binfmt_elf.c doesn't handle
>>>> regset->active() returning < 0.  Guess that may be why.  Looks like
>>>> something that could be cleaned up, to me.
>>>>
>> Also it does not consider the return value of regset->active(t->task, regset)
>> (whose objective is to figure out whether we need to request regset->n number
>> of elements or less than that) in the subsequent call to regset->get function.
> 
> Indeed.
> 
> TBC, do you plan on fixing this?  Otherwise ...

Sure, thinking something like this as mentioned below. But still not sure how to use
the return type of -ENODEV from the function regset->active(). Right now if any
regset does have the active hook and it returns anything but positive value, it will
be ignored and the control moves to the next regset in view. This prevents the thread
core note type being written to the core dump.

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index aa3cb62..80672fb 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1553,7 +1553,15 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
                if (regset->core_note_type && regset->get &&
                    (!regset->active || regset->active(t->task, regset))) {
                        int ret;
-                       size_t size = regset->n * regset->size;
+                       size_t size;
+
+                       /* Request only the active elements in the regset */
+                       if (!regset->active)
+                               size = regset->n * regset->size;
+                       else
+                               size = regset->active(t->task, regset)
+                                                               * regset->size;
+
                        void *data = kmalloc(size, GFP_KERNEL);
                        if (unlikely(!data))
                                return 0;

> 
>> Now coming to the installation of the .active hooks part for all the new regsets, it
>> should be pretty straight forward as well. Though its optional and used for elf_core_dump
>> purpose only, its worth adding them here. Example of an active function should be something
>> like this. The function is inexpensive as required.
>>
>> +static int tm_spr_active(struct task_struct *target,
>> +                               const struct user_regset *regset)
>> +{
>> +       if (!cpu_has_feature(CPU_FTR_TM))
>> +               return -ENODEV;
> 
> ... unfortunately this will do the wrong thing.

I am not sure whether I understand this correctly. Are you saying that its wrong to return
-ENODEV in this case as above ?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/