RE: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops

[Nixiaoming]

On 2019/9/19 17:30, Greg KH wrote:
> On Thu, Sep 19, 2019 at 05:18:15PM +0800, Xiaoming Ni wrote:
>> Using kzalloc() to allocate memory in function con_init(), but not
>> checking the return value, there is a risk of null pointer references
>> oops.
>>
>> Signed-off-by: Xiaoming Ni 
>
> We keep having this be "reported" 
>
>> ---
>>  drivers/tty/vt/vt.c | 18 ++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
>> index 34aa39d..db83e52 100644
>> --- a/drivers/tty/vt/vt.c
>> +++ b/drivers/tty/vt/vt.c
>> @@ -3357,15 +3357,33 @@ static int __init con_init(void)
>>  
>>  for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
>>  vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), 
>> GFP_NOWAIT);
>> +if (unlikely(!vc)) {
>> +pr_warn("%s:failed to allocate memory for the %u vc\n",
>> +__func__, currcons);
>> +break;
>> +}
>
> At init, this really can not happen.  Have you see it ever happen?

I did not actually observe the null pointer here.
I am confused when I see the code allocated here without check the return value.
Small memory allocation failures are difficult to occur during system 
initialization
But is it not safe enough if the code is not judged?
Also if the memory allocation failure is not allowed here, is it better to add 
the __GFP_NOFAIL flags?

>>  INIT_WORK(_cons[currcons].SAK_work, vc_SAK);
>>  tty_port_init(>port);
>>  visual_init(vc, currcons, 1);
>>  vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
>> +if (unlikely(!vc->vc_screenbuf)) {
>
> Never use likely/unlikely unless you can actually measure the speed
> difference.  For something like this, the compiler will always get it
> right without you having to do anything.
>
Thank you for your guidance.
 I misuse it.

> And again, how can this fail?  Have you seen it fail?
>
>> +pr_warn("%s:failed to allocate memory for the %u 
>> vc_screenbuf\n",
>> +__func__, currcons);
>> +visual_deinit(vc);
>> +tty_port_destroy(>port);
>> +kfree(vc);
>> +vc_cons[currcons].d = NULL;
>> +break;
>> +}
>>  vc_init(vc, vc->vc_rows, vc->vc_cols,
>>  currcons || !vc->vc_sw->con_save_screen);
>>  }
>>  currcons = fg_console = 0;
>>  master_display_fg = vc = vc_cons[currcons].d;
>> +if (unlikely(!vc)) {
>
> Again, never use likely/unlikely unless you can measure it.
>
> thanks,
>
> greg k-h
>
> .
>
thanks,
Xiaoming Ni

RE: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration

[Nixiaoming]

On Wed, July 10, 2019 1:49 PM Vasily Averin wrote:
>On 7/10/19 6:09 AM, Xiaoming Ni wrote:
>> Registering the same notifier to a hook repeatedly can cause the hook
>> list to form a ring or lose other members of the list.
>
>I think is not enough to _prevent_ 2nd register attempt,
>it's enough to detect just attempt and generate warning to mark host in bad 
>state.
>

Duplicate registration is prevented in my patch, not just "mark host in bad 
state"

Duplicate registration is checked and exited in notifier_chain_cond_register()

Duplicate registration was checked in notifier_chain_register() but only 
the alarm was triggered without exiting. added by commit 831246570d34692e 
("kernel/notifier.c: double register detection")

My patch is like a combination of 831246570d34692e and 
notifier_chain_cond_register(),
 which triggers an alarm and exits when a duplicate registration is detected.

>Unexpected 2nd register of the same hook most likely will lead to 2nd 
>unregister,
>and it can lead to host crash in any time: 
>you can unregister notifier on first attempt it can be too early, it can be 
>still in use.
>on the other hand you can never call 2nd unregister at all.

Since the member was not added to the linked list at the time of the second 
registration, 
no linked list ring was formed. 
The member is released on the first unregistration and -ENOENT on the second 
unregistration.
After patching, the fault has been alleviated

It may be more helpful to return an error code when someone tries to register 
the same
notification program a second time.
But I noticed that notifier_chain_cond_register() returns 0 when duplicate 
registration 
is detected. At the same time, in all the existing export function comments of 
notify,
"Currently always returns zero"

I am a bit confused: which is better?

>
>Unfortunately I do not see any ways to handle such cases properly,
>and it seems for me your patches does not resolve this problem.
>
>Am I missed something probably?
> 
>> case1: An infinite loop in notifier_chain_register() can cause soft lockup
>> atomic_notifier_chain_register(_notifier_list, );
>> atomic_notifier_chain_register(_notifier_list, );
>> atomic_notifier_chain_register(_notifier_list, );

Thanks

Xiaoming Ni

RE: [PATCH v3 0/3] kernel/notifier.c: avoid duplicate registration

[Nixiaoming]

On Wed, July 10, 2019 1:56 PM Greg KH wrote:
>On Wed, Jul 10, 2019 at 11:09:07AM +0800, Xiaoming Ni wrote:
>> Registering the same notifier to a hook repeatedly can cause the hook
>> list to form a ring or lose other members of the list.
>
>Then don't do that :)
>

Duplicate registration is checked and exited in notifier_chain_cond_register()

Duplicate registration was checked in notifier_chain_register() but only 
the alarm was triggered without exiting. added by commit 831246570d34692e 
("kernel/notifier.c: double register detection")

This patch is similar to commit 8312465 and notifier_chain_cond_register(),
 with actual prevention for such behaviour,  which I think is necessary to 
 avoid the formation of a linked list ring.

>Is there any in-kernel users that do do this?  If so, please just fix
>them.
>
Notifier_chain_register() is not a hotspot path.
Adding a check here can make the kernel more stable.

Thanks

Xiaoming Ni


>thanks,
>
>greg k-h
>

RE: [PATCH] kernel/notifier.c: remove notifier_chain_register

[Nixiaoming]

On Fri, 14 Jun 2019 03:38 AM Andrew Morton  wrote:
>On Thu, 13 Jun 2019 22:07:44 +0800 Xiaoming Ni  wrote:
>
>> Registering the same notifier to a hook repeatedly can cause the hook
>> list to form a ring or lose other members of the list.
>> .
>> 
>> diff --git a/kernel/notifier.c b/kernel/notifier.c
>> index d9f5081..56efd54 100644
>> --- a/kernel/notifier.c
>> +++ b/kernel/notifier.c
>> @@ -19,20 +19,6 @@
>>   *  are layered on top of these, with appropriate locking added.
>>   */
>>  
>> -static int notifier_chain_register(struct notifier_block **nl,
>> -struct notifier_block *n)
>> -{
>> -while ((*nl) != NULL) {
>> -WARN_ONCE(((*nl) == n), "double register detected");
>> -if (n->priority > (*nl)->priority)
>> -break;
>> -nl = &((*nl)->next);
>> -}
>> -n->next = *nl;
>> -rcu_assign_pointer(*nl, n);
>> -return 0;
>> -}
>
>Registering an already-registered notifier is a bug (except for in
>net/sunrpc/rpc_pipe.c, apparently).  The effect of this change is to
>remove the warning about the presence of the bug, so the bug is less
>likely to get fixed.
>
thanks for your guidance,

Should I modify this way 
   1 notifier_chain_cond_register() and notifier_chain_register() should be 
combined into one function.
   2 The warning information needs to be displayed while prohibiting duplicate 
registration.
@@ -23,7 +23,10 @@ static int notifier_chain_register(struct 
notifier_block **nl,
struct notifier_block *n)
 {
while ((*nl) != NULL) {
-   WARN_ONCE(((*nl) == n), "double register detected");
+ if (unlikely((*nl) == n)) {
+ WARN(1, "double register detected");
+ return 0;
+ }
if (n->priority > 
(*nl)->priority)
break;

>I think it would be better to remove notifier_chain_cond_register() and
>blocking_notifier_chain_cond_register() and to figure out why
>net/sunrpc/rpc_pipe.c is using it and to redo the rpc code so it no
>longer has that need.
>
thanks for your guidance,
I re-examine the submission record and analyze it as follows

notifier_chain_cond_register() was introduced by commit 6546bc4279241e8fa43
 ("ipc: re-enable msgmni automatic recomputing msgmni if ​​set to negative")
From the patch description information, it should be done to avoid repeated 
registrations,
 but I don't know why not directly modify notifier_chain_cond_register().
 
notifier_chain_cond_register() is only called by 
blocking_notifier_chain_cond_register()
blocking_notifier_chain_cond_register() has less processing of the 
SYSTEM_BOOTING state 
than blocking_notifier_chain_egister().
may also be a bug.

ipc/ipcns_notifier.c and the call to blocking_notifier_chain_cond_register() 
are removed 
in commit 0050ee059f7fc86b1df252 ("ipc/msg: increase MSGMNI, remove scaling").

now blocking_notifier_chain_cond_register() is only used in 
net/sunrpc/rpc_pipe.c, 
commit 2d00131acc641b2cb6 ("SUNRPC: send notification events on pipefs sb 
creation and destruction")
Using blocking_notifier_chain_cond_register() may also be to avoid duplicate 
registrations??

thanks

RE: [PATCH v3 2/2] clk:mmp: clk-mix.c fix divide-by-zero

[Nixiaoming]

On Wed, Apr 24, 2019 at 7:00 AM Stephen Boyd  wrote:
>Quoting nixiaoming (2019-03-30 06:55:42)
>> The _get_div() function has a branch with a return value of 0
>> Add a check on the return value of _get_div() to avoid divide-by-zero
>> 
>
>Are you seeing this in practice? Or just trying to avoid a div-by-zero
>case that you've found from inspection? 
>
This potential bug is found by code inspection.
_get_div() is defined as a static function which is only refered twice 
in drivers/clk/mmp/clk-mix.c. In both cases the return value of _get_div()
 is used as divider without any check.
 If _get_div() never returns 0, then the branch returning 0 is dead code,
or the return value should be check to avoid dividing by zero error. 

thanks

>> Signed-off-by: nixiaoming 
>> Reviewed-by: Mukesh Ojha 
>

RE: [PATCH v3 1/2] clk:Fix divide-by-zero in divider_ro_round_rate_parent

[Nixiaoming]

On Wed, Apr 24, 2019 at 6:52 AM Stephen Boyd  wrote:
>Quoting nixiaoming (2019-03-30 06:54:50)
>> In the function divider_recalc_rate() The judgment of the return value of
>> _get_div() indicates that the return value of _get_div() can be 0.
>
>When does _get_div() return 0? It can't be CLK_DIVIDER_MAX_AT_ZERO or
>CLK_DIVIDER_POWER_OF_TWO. I suppose it could be CLK_DIVIDER_ONE_BASED if
>CLK_DIVIDER_ALLOW_ZERO is set? Or just CLK_DIVIDER_ALLOW_ZERO is set? Or
>a table that has 0 in it for some odd reason.
>
divider_ro_round_rate_parent() is an exported function.
There is no parameter check or return value check before 
and after calling _get_div(), which may result in a divide by zero error.

Case1: The "flags" contains CLK_DIVIDER_ONE_BASED, and "val" is 0.
Case2: The "flags" does not contain CLK_DIVIDER_ONE_BASED, 
CLK_DIVIDER_POWER_OF_TWO, CLK_DIVIDER_MAX_AT_ZERO,  
"table" is NULL.  "val" is 0x
In both cases _get_div() returns 0

>> In order to avoid the divide-by-zero error, add check for return value
>> of _get_div() in the divider_ro_round_rate_parent()
>> 
>> Signed-off-by: nixiaoming 
>> Reviewed-by: Mukesh Ojha 
>> ---
>>  drivers/clk/clk-divider.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
>> index e5a1726..f4bf7a4 100644
>> --- a/drivers/clk/clk-divider.c
>> +++ b/drivers/clk/clk-divider.c
>> @@ -347,6 +347,9 @@ long divider_ro_round_rate_parent(struct clk_hw *hw, 
>> struct clk_hw *parent,
>> int div;
>>  
>> div = _get_div(table, val, flags, width);
>> +   /* avoid divide-by-zero */
>> +   if (!div)
>> +   return -EINVAL;
>
>Can you please give more details on what's happening here? Who's the
>caller? What are the arguments being passed in? Shouldn't we check for
>CLK_DIVIDER_ALLOW_ZERO and then return prate as it comes in instead of
>returning an error?
>
I found that there may be a divide-by-zero error by code review,
 for example: "flags" is CLK_DIVIDER_ONE_BASED and "val" is 0.
So simply add a return value check to avoid divide-by-zero

thanks for your suggestion, 
I will resend the patch later
refer to your advice and divider_recalc_rate() to add a check for 
CLK_DIVIDER_ALLOW_ZERO

thanks

[PATCH v3 2/2] clk:mmp: clk-mix.c fix divide-by-zero

[nixiaoming]

The _get_div() function has a branch with a return value of 0
Add a check on the return value of _get_div() to avoid divide-by-zero

Signed-off-by: nixiaoming 
Reviewed-by: Mukesh Ojha 
---
 drivers/clk/mmp/clk-mix.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/mmp/clk-mix.c b/drivers/clk/mmp/clk-mix.c
index 90814b2..6ed5ad7 100644
--- a/drivers/clk/mmp/clk-mix.c
+++ b/drivers/clk/mmp/clk-mix.c
@@ -245,6 +245,9 @@ static int mmp_clk_mix_determine_rate(struct clk_hw *hw,
div_val_max = _get_maxdiv(mix);
for (j = 0; j < div_val_max; j++) {
div = _get_div(mix, j);
+   /* avoid divide-by-zero */
+   if (!div)
+   continue;
mix_rate = parent_rate / div;
gap = abs(mix_rate - req->rate);
if (!parent_best || gap < gap_best) {
@@ -341,6 +344,9 @@ static unsigned long mmp_clk_mix_recalc_rate(struct clk_hw 
*hw,
shift = mix->reg_info.shift_div;
 
div = _get_div(mix, MMP_CLK_BITS_GET_VAL(mux_div, width, shift));
+   /* avoid divide-by-zero */
+   if (!div)
+   return -EINVAL;
 
return parent_rate / div;
 }
-- 
1.8.5.6

[PATCH v3 1/2] clk:Fix divide-by-zero in divider_ro_round_rate_parent

[nixiaoming]

In the function divider_recalc_rate() The judgment of the return value of
_get_div() indicates that the return value of _get_div() can be 0.
In order to avoid the divide-by-zero error, add check for return value
of _get_div() in the divider_ro_round_rate_parent()

Signed-off-by: nixiaoming 
Reviewed-by: Mukesh Ojha 
---
 drivers/clk/clk-divider.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index e5a1726..f4bf7a4 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -347,6 +347,9 @@ long divider_ro_round_rate_parent(struct clk_hw *hw, struct 
clk_hw *parent,
int div;
 
div = _get_div(table, val, flags, width);
+   /* avoid divide-by-zero */
+   if (!div)
+   return -EINVAL;
 
/* Even a read-only clock can propagate a rate change */
if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
-- 
1.8.5.6

RE: [PATCH v2 1/2] clk:Fix divide-by-zero in divider_ro_round_rate_parent

[Nixiaoming]

On 3/30/2019 5:44 PM, Mukesh Ojha wrote:
>On 3/30/2019 8:01 AM, nixiaoming wrote:
>> In the function divider_recalc_rate() The judgment of the return value of
>> _get_div() indicates that the return value of _get_div() may be 0.
>
>s/may be/can be

Thank you for your comments.
I will fix it in a later patch.
>
>> In order to avoid the divide-by-zero error, add check the return values.
>s/add check the/add check for
>
Thank you for your comments.
I will fix it in a later patch.

>> of _get_div() in the divider_ro_round_rate_parent()
>>
>> Signed-off-by: nixiaoming 
>
>
>Please fix the commit text.
>Reviewed-by: Mukesh Ojha 
>
>Cheers,
>-Mukesh
>

Thanks

[PATCH v2 2/2] clk:mmp: clk-mix.c fix divide-by-zero

[nixiaoming]

The _get_div() function has a branch with a return value of 0
Add a check on the return value of _get_div() to avoid divide-by-zero

Signed-off-by: nixiaoming 
Reviewed-by: Mukesh Ojha 
---
 drivers/clk/mmp/clk-mix.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/mmp/clk-mix.c b/drivers/clk/mmp/clk-mix.c
index 90814b2..6ed5ad7 100644
--- a/drivers/clk/mmp/clk-mix.c
+++ b/drivers/clk/mmp/clk-mix.c
@@ -245,6 +245,9 @@ static int mmp_clk_mix_determine_rate(struct clk_hw *hw,
div_val_max = _get_maxdiv(mix);
for (j = 0; j < div_val_max; j++) {
div = _get_div(mix, j);
+   /* avoid divide-by-zero */
+   if (!div)
+   continue;
mix_rate = parent_rate / div;
gap = abs(mix_rate - req->rate);
if (!parent_best || gap < gap_best) {
@@ -341,6 +344,9 @@ static unsigned long mmp_clk_mix_recalc_rate(struct clk_hw 
*hw,
shift = mix->reg_info.shift_div;
 
div = _get_div(mix, MMP_CLK_BITS_GET_VAL(mux_div, width, shift));
+   /* avoid divide-by-zero */
+   if (!div)
+   return -EINVAL;
 
return parent_rate / div;
 }
-- 
1.8.5.6

[PATCH v2 1/2] clk:Fix divide-by-zero in divider_ro_round_rate_parent

[nixiaoming]

In the function divider_recalc_rate() The judgment of the return value of
_get_div() indicates that the return value of _get_div() may be 0.
In order to avoid the divide-by-zero error, add check the return value
of _get_div() in the divider_ro_round_rate_parent()

Signed-off-by: nixiaoming 
---
 drivers/clk/clk-divider.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index e5a1726..f4bf7a4 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -347,6 +347,9 @@ long divider_ro_round_rate_parent(struct clk_hw *hw, struct 
clk_hw *parent,
int div;
 
div = _get_div(table, val, flags, width);
+   /* avoid divide-by-zero */
+   if (!div)
+   return -EINVAL;
 
/* Even a read-only clock can propagate a rate change */
if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
-- 
1.8.5.6

RE: [PATCH] clk:mmp: clk-mix.c fix divide-by-zero

[Nixiaoming]

On 3/30/2019 6:48 AM Stephen Boyd wrote:
>Quoting nixiaoming (2019-03-29 04:46:00)
>> The _get_div function has a branch with a return value of 0
>> Add a check on the return value of _get_div to avoid divide-by-zero
>> 
>> Signed-off-by: nixiaoming 
>
>Similar questions apply here as they do on the generic divider patch you
>sent.
>
_get_div() in both files is a different function, with a divide-by-zero problem
I will organize it into a patch set later.

>> ---
>>  drivers/clk/mmp/clk-mix.c | 4 
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/clk/mmp/clk-mix.c b/drivers/clk/mmp/clk-mix.c
>> index 90814b2..9d152c2 100644
>> --- a/drivers/clk/mmp/clk-mix.c
>> +++ b/drivers/clk/mmp/clk-mix.c
>> @@ -245,6 +245,8 @@ static int mmp_clk_mix_determine_rate(struct clk_hw *hw,
>> div_val_max = _get_maxdiv(mix);
>> for (j = 0; j < div_val_max; j++) {
>> div = _get_div(mix, j);
>> +   if (!div) /* avoid divide-by-zero */
>
>Why can't we return 1 for the divider value here?

I personally understand that an exception or skip should be thrown after 
dividing by 0.
Directly modified to other values, I am not sure whether it affects the logic
My logical understanding of this code is not clear enough, I still need your 
guidance.
>
>> +   continue;
>> mix_rate = parent_rate / div;
>> gap = abs(mix_rate - req->rate);
>> if (!parent_best || gap < gap_best) {
>> @@ -341,6 +343,8 @@ static unsigned long mmp_clk_mix_recalc_rate(struct 
>> clk_hw *hw,
>> shift = mix->reg_info.shift_div;
>>  
>> div = _get_div(mix, MMP_CLK_BITS_GET_VAL(mux_div, width, shift));
>> +   if (!div) /* avoid divide-by-zero */
>
>Same question.
I personally understand that an exception or skip should be thrown after 
dividing by 0.
Directly modified to other values, I am not sure whether it affects the logic
My logical understanding of this code is not clear enough, I still need your 
guidance.
>
>> +   return -EINVAL;
>>  
>> return parent_rate / div;
>>  }
>

RE: [PATCH] clk:Fix divide by 0 error in divider_ro_round_rate_parent

[Nixiaoming]

On 3/30/2019 6:42 AM Stephen Boyd wrote:
>Quoting nixiaoming (2019-03-29 02:05:24)
>> In the function divider_recalc_rate The judgment of the return value of
>
>Please write divider_recalc_rate() with parenthesis to show it's a
>function.
>
>> _get_div indicates that the return value of _get_div may be 0.
>
>__get_div()
Thank you for your guidance, I will correct it later in the patch.

>> In order to avoid the divide-by-zero error, add check the return value
>> of _get_div in the divider_ro_round_rate_parent
>> 
>> Signed-off-by: nixiaoming 
>
>Is this your name? nixiaoming? Or is it written some other way?

Chinese name 倪小明
>> ---
>>  drivers/clk/clk-divider.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
>> index e5a1726..0854e3e 100644
>> --- a/drivers/clk/clk-divider.c
>> +++ b/drivers/clk/clk-divider.c
>> @@ -347,6 +347,8 @@ long divider_ro_round_rate_parent(struct clk_hw *hw, 
>> struct clk_hw *parent,
>> int div;
>>  
>> div = _get_div(table, val, flags, width);
>> +   if (!div) /* avoid divide-by-zero */
>> +   return -EINVAL;
>
>How does _get_div() return 0? What is the value of 'flags' here when
>this goes wrong?

divider_ro_round_rate_parent() and divider_recalc_rate() are functions
 of the EXPORT_SYMBOL_GPL attribute

If _get_div() can return 0 in the argument of divider_recalc_rate()
Then should be able to return 0 in divider_ro_round_rate_parent()

>
>>  
>> /* Even a read-only clock can propagate a rate change */
>> if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
>> -- 
>> 1.8.5.6
>> 
>
>Wow that's a 5 year old version of git!
>
>

[PATCH v2] clk:mmp: clk-mix.c fix divide-by-zero

[nixiaoming]

The _get_div function has a branch with a return value of 0
Add a check on the return value of _get_div to avoid divide-by-zero

Signed-off-by: nixiaoming 
Reviewed-by: Mukesh Ojha 
---
 drivers/clk/mmp/clk-mix.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/mmp/clk-mix.c b/drivers/clk/mmp/clk-mix.c
index 90814b2..6ed5ad7 100644
--- a/drivers/clk/mmp/clk-mix.c
+++ b/drivers/clk/mmp/clk-mix.c
@@ -245,6 +245,9 @@ static int mmp_clk_mix_determine_rate(struct clk_hw *hw,
div_val_max = _get_maxdiv(mix);
for (j = 0; j < div_val_max; j++) {
div = _get_div(mix, j);
+   /* avoid divide-by-zero */
+   if (!div)
+   continue;
mix_rate = parent_rate / div;
gap = abs(mix_rate - req->rate);
if (!parent_best || gap < gap_best) {
@@ -341,6 +344,9 @@ static unsigned long mmp_clk_mix_recalc_rate(struct clk_hw 
*hw,
shift = mix->reg_info.shift_div;
 
div = _get_div(mix, MMP_CLK_BITS_GET_VAL(mux_div, width, shift));
+   /* avoid divide-by-zero */
+   if (!div)
+   return -EINVAL;
 
return parent_rate / div;
 }
-- 
1.8.5.6

[PATCH] clk:mmp: clk-mix.c fix divide-by-zero

[nixiaoming]

The _get_div function has a branch with a return value of 0
Add a check on the return value of _get_div to avoid divide-by-zero

Signed-off-by: nixiaoming 
---
 drivers/clk/mmp/clk-mix.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/mmp/clk-mix.c b/drivers/clk/mmp/clk-mix.c
index 90814b2..9d152c2 100644
--- a/drivers/clk/mmp/clk-mix.c
+++ b/drivers/clk/mmp/clk-mix.c
@@ -245,6 +245,8 @@ static int mmp_clk_mix_determine_rate(struct clk_hw *hw,
div_val_max = _get_maxdiv(mix);
for (j = 0; j < div_val_max; j++) {
div = _get_div(mix, j);
+   if (!div) /* avoid divide-by-zero */
+   continue;
mix_rate = parent_rate / div;
gap = abs(mix_rate - req->rate);
if (!parent_best || gap < gap_best) {
@@ -341,6 +343,8 @@ static unsigned long mmp_clk_mix_recalc_rate(struct clk_hw 
*hw,
shift = mix->reg_info.shift_div;
 
div = _get_div(mix, MMP_CLK_BITS_GET_VAL(mux_div, width, shift));
+   if (!div) /* avoid divide-by-zero */
+   return -EINVAL;
 
return parent_rate / div;
 }
-- 
1.8.5.6

[PATCH] clk:Fix divide by 0 error in divider_ro_round_rate_parent

[nixiaoming]

In the function divider_recalc_rate The judgment of the return value of
_get_div indicates that the return value of _get_div may be 0.
In order to avoid the divide-by-zero error, add check the return value
of _get_div in the divider_ro_round_rate_parent

Signed-off-by: nixiaoming 
---
 drivers/clk/clk-divider.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index e5a1726..0854e3e 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -347,6 +347,8 @@ long divider_ro_round_rate_parent(struct clk_hw *hw, struct 
clk_hw *parent,
int div;
 
div = _get_div(table, val, flags, width);
+   if (!div) /* avoid divide-by-zero */
+   return -EINVAL;
 
/* Even a read-only clock can propagate a rate change */
if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
-- 
1.8.5.6

RE: [PATCH v2] fanotify reports the thread id of the event trigger

[Nixiaoming]

On Tue, Sep 18, 2018 3:07 PM Amir Goldstein 
>On Tue, Sep 18, 2018 at 6:01 AM Nixiaoming  wrote:
>>
>> On Mon, Sep 17, 2018 11:51 PM Amir Goldstein  wrote:
>> >On Mon, Sep 17, 2018 at 6:05 PM nixiaoming  wrote:
>...
>> >> diff --git a/include/linux/fsnotify_backend.h 
>> >> b/include/linux/fsnotify_backend.h
>> >> index b8f4182..44c659f 100644
>> >> --- a/include/linux/fsnotify_backend.h
>> >> +++ b/include/linux/fsnotify_backend.h
>> >> @@ -193,6 +193,7 @@ struct fsnotify_group {
>> >> unsigned int max_marks;
>> >> struct user_struct *user;
>> >> bool audit;
>> >> +   bool should_report_tid;
>> >
>> >For brevity I would call that report_tid, but not insisting.
>> >
>>
>> Whether it is better to change to "unsigned int flags"
>> Save "group->fanotify_data.flags=flags;" in "fanotify_init"
>> Determine whether "group->fanotify_data.flags" contains "FAN_EVENT_INFO_TID" 
>> in "fanotify_alloc_event";
>>
>> At the same time, if there are other flags that need to be used later, there 
>> is no need to add new members.
>>
>> By the way, whether or not "bool audit" can also be included by flags
>>
>
>I strongly agree. Didn't want to impose this change on you, but in
>fact, I already did
>that in my own patch set, so you can use my patches as reference:
>https://github.com/amir73il/linux/commit/5225fe1e19c74f4d7a4a4cc98ff6ef5872c8e620
>https://github.com/amir73il/linux/commit/0d1a226f5c18012f19ed6eabfab46f0f125ec190
>
>If you do this you need to separate your change into 2 patches.
>First make the fanotify_data.flags change collecting the pieces from both
>my patches.
>Then make your TID change using fanotify_data.flags.
>
>Thanks,
>Amir.
>
Should I do this:
1 git cherry-pick 5225fe1e19c74f4d7a4a4cc98ff6ef5872c8e620 
Git cherry-pick 0d1a226f5c18012f19ed6eabfab46f0f125ec190
  Manually handle patch conflicts 
2 Add a new flag "FAN_EVENT_INFO_TID" into fanotify_data.flags
3 Incorporate "bool audit" into fanotify_data.flags
4 Submit 4 patches

thanks

RE: [PATCH v2] fanotify reports the thread id of the event trigger

[Nixiaoming]

On Tue, Sep 18, 2018 3:07 PM Amir Goldstein 
>On Tue, Sep 18, 2018 at 6:01 AM Nixiaoming  wrote:
>>
>> On Mon, Sep 17, 2018 11:51 PM Amir Goldstein  wrote:
>> >On Mon, Sep 17, 2018 at 6:05 PM nixiaoming  wrote:
>...
>> >> diff --git a/include/linux/fsnotify_backend.h 
>> >> b/include/linux/fsnotify_backend.h
>> >> index b8f4182..44c659f 100644
>> >> --- a/include/linux/fsnotify_backend.h
>> >> +++ b/include/linux/fsnotify_backend.h
>> >> @@ -193,6 +193,7 @@ struct fsnotify_group {
>> >> unsigned int max_marks;
>> >> struct user_struct *user;
>> >> bool audit;
>> >> +   bool should_report_tid;
>> >
>> >For brevity I would call that report_tid, but not insisting.
>> >
>>
>> Whether it is better to change to "unsigned int flags"
>> Save "group->fanotify_data.flags=flags;" in "fanotify_init"
>> Determine whether "group->fanotify_data.flags" contains "FAN_EVENT_INFO_TID" 
>> in "fanotify_alloc_event";
>>
>> At the same time, if there are other flags that need to be used later, there 
>> is no need to add new members.
>>
>> By the way, whether or not "bool audit" can also be included by flags
>>
>
>I strongly agree. Didn't want to impose this change on you, but in
>fact, I already did
>that in my own patch set, so you can use my patches as reference:
>https://github.com/amir73il/linux/commit/5225fe1e19c74f4d7a4a4cc98ff6ef5872c8e620
>https://github.com/amir73il/linux/commit/0d1a226f5c18012f19ed6eabfab46f0f125ec190
>
>If you do this you need to separate your change into 2 patches.
>First make the fanotify_data.flags change collecting the pieces from both
>my patches.
>Then make your TID change using fanotify_data.flags.
>
>Thanks,
>Amir.
>
Should I do this:
1 git cherry-pick 5225fe1e19c74f4d7a4a4cc98ff6ef5872c8e620 
Git cherry-pick 0d1a226f5c18012f19ed6eabfab46f0f125ec190
  Manually handle patch conflicts 
2 Add a new flag "FAN_EVENT_INFO_TID" into fanotify_data.flags
3 Incorporate "bool audit" into fanotify_data.flags
4 Submit 4 patches

thanks

RE: [PATCH v2] fanotify reports the thread id of the event trigger

[Nixiaoming]

On Mon, Sep 17, 2018 11:51 PM Amir Goldstein  wrote:
>On Mon, Sep 17, 2018 at 6:05 PM nixiaoming  wrote:
>>
>> In order to identify which thread triggered the event in the
>> multi-threaded program, add the FAN_EVENT_INFO_TID tag in fanotify_init
>
>According to code and man page this is a 'flag' not a 'tag'
>Please stick to existing terminology.
>
Thank you for your guidance, I will correct it in a later patch.

>> to select whether to report the event creator's thread id information.
>>
>> Signed-off-by: nixiaoming 
...
>> @@ -693,9 +693,9 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, 
>> unsigned int, event_f_flags)
>> return -EPERM;
>>
>>  #ifdef CONFIG_AUDITSYSCALL
>> -   if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_ENABLE_AUDIT))
>> +   if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_ENABLE_AUDIT | 
>> FAN_EVENT_INFO_TID))
>>  #else
>> -   if (flags & ~FAN_ALL_INIT_FLAGS)
>> +   if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_EVENT_INFO_TID))
>
>Not this way. You need to add FAN_EVENT_INFO_TID to FAN_ALL_INIT_FLAGS.
Thank you for your guidance, I will correct it in a later patch.

>
>>  #endif
>> return -EINVAL;
>>
>> @@ -731,6 +731,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, 
>> unsigned int, event_f_flags)
>> }
>>
>> group->fanotify_data.user = user;
>> +   group->fanotify_data.should_report_tid = (flags & 
>> FAN_EVENT_INFO_TID) ? true : false;
>
>Please drop "? true : false" its not needed.
>
>> atomic_inc(>fanotify_listeners);
>> group->memcg = get_mem_cgroup_from_mm(current->mm);
>>
>> diff --git a/include/linux/fsnotify_backend.h 
>> b/include/linux/fsnotify_backend.h
>> index b8f4182..44c659f 100644
>> --- a/include/linux/fsnotify_backend.h
>> +++ b/include/linux/fsnotify_backend.h
>> @@ -193,6 +193,7 @@ struct fsnotify_group {
>> unsigned int max_marks;
>> struct user_struct *user;
>> bool audit;
>> +   bool should_report_tid;
>
>For brevity I would call that report_tid, but not insisting.
>

Whether it is better to change to "unsigned int flags"
Save "group->fanotify_data.flags=flags;" in "fanotify_init"
Determine whether "group->fanotify_data.flags" contains "FAN_EVENT_INFO_TID" in 
"fanotify_alloc_event";

At the same time, if there are other flags that need to be used later, there is 
no need to add new members.

By the way, whether or not "bool audit" can also be included by flags

thanks


>> } fanotify_data;
>>  #endif /* CONFIG_FANOTIFY */
>> };
>> diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
>> index 7424791..4eba41c 100644
>> --- a/include/uapi/linux/fanotify.h
>> +++ b/include/uapi/linux/fanotify.h
>> @@ -18,6 +18,7 @@
>>
>>  #define FAN_ONDIR  0x4000  /* event occurred against 
>> dir */
>>
>> +#define FAN_EVENT_INFO_TID 0x0200  /* reports the thread id of 
>> the event trigger */
>
>That is not the right place nor the sensible value for this flag, but this:
>
>#define FAN_UNLIMITED_QUEUE 0x0010
>#define FAN_UNLIMITED_MARKS 0x0020
>#define FAN_ENABLE_AUDIT0x0040
>+#define FAN_EVENT_INFO_TID 0x0080
>
>Thanks,
>Amir.
>

RE: [PATCH v2] fanotify reports the thread id of the event trigger

[Nixiaoming]

On Mon, Sep 17, 2018 11:51 PM Amir Goldstein  wrote:
>On Mon, Sep 17, 2018 at 6:05 PM nixiaoming  wrote:
>>
>> In order to identify which thread triggered the event in the
>> multi-threaded program, add the FAN_EVENT_INFO_TID tag in fanotify_init
>
>According to code and man page this is a 'flag' not a 'tag'
>Please stick to existing terminology.
>
Thank you for your guidance, I will correct it in a later patch.

>> to select whether to report the event creator's thread id information.
>>
>> Signed-off-by: nixiaoming 
...
>> @@ -693,9 +693,9 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, 
>> unsigned int, event_f_flags)
>> return -EPERM;
>>
>>  #ifdef CONFIG_AUDITSYSCALL
>> -   if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_ENABLE_AUDIT))
>> +   if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_ENABLE_AUDIT | 
>> FAN_EVENT_INFO_TID))
>>  #else
>> -   if (flags & ~FAN_ALL_INIT_FLAGS)
>> +   if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_EVENT_INFO_TID))
>
>Not this way. You need to add FAN_EVENT_INFO_TID to FAN_ALL_INIT_FLAGS.
Thank you for your guidance, I will correct it in a later patch.

>
>>  #endif
>> return -EINVAL;
>>
>> @@ -731,6 +731,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, 
>> unsigned int, event_f_flags)
>> }
>>
>> group->fanotify_data.user = user;
>> +   group->fanotify_data.should_report_tid = (flags & 
>> FAN_EVENT_INFO_TID) ? true : false;
>
>Please drop "? true : false" its not needed.
>
>> atomic_inc(>fanotify_listeners);
>> group->memcg = get_mem_cgroup_from_mm(current->mm);
>>
>> diff --git a/include/linux/fsnotify_backend.h 
>> b/include/linux/fsnotify_backend.h
>> index b8f4182..44c659f 100644
>> --- a/include/linux/fsnotify_backend.h
>> +++ b/include/linux/fsnotify_backend.h
>> @@ -193,6 +193,7 @@ struct fsnotify_group {
>> unsigned int max_marks;
>> struct user_struct *user;
>> bool audit;
>> +   bool should_report_tid;
>
>For brevity I would call that report_tid, but not insisting.
>

Whether it is better to change to "unsigned int flags"
Save "group->fanotify_data.flags=flags;" in "fanotify_init"
Determine whether "group->fanotify_data.flags" contains "FAN_EVENT_INFO_TID" in 
"fanotify_alloc_event";

At the same time, if there are other flags that need to be used later, there is 
no need to add new members.

By the way, whether or not "bool audit" can also be included by flags

thanks


>> } fanotify_data;
>>  #endif /* CONFIG_FANOTIFY */
>> };
>> diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
>> index 7424791..4eba41c 100644
>> --- a/include/uapi/linux/fanotify.h
>> +++ b/include/uapi/linux/fanotify.h
>> @@ -18,6 +18,7 @@
>>
>>  #define FAN_ONDIR  0x4000  /* event occurred against 
>> dir */
>>
>> +#define FAN_EVENT_INFO_TID 0x0200  /* reports the thread id of 
>> the event trigger */
>
>That is not the right place nor the sensible value for this flag, but this:
>
>#define FAN_UNLIMITED_QUEUE 0x0010
>#define FAN_UNLIMITED_MARKS 0x0020
>#define FAN_ENABLE_AUDIT0x0040
>+#define FAN_EVENT_INFO_TID 0x0080
>
>Thanks,
>Amir.
>

[PATCH v2] fanotify reports the thread id of the event trigger

[nixiaoming]

In order to identify which thread triggered the event in the
multi-threaded program, add the FAN_EVENT_INFO_TID tag in fanotify_init
to select whether to report the event creator's thread id information.

Signed-off-by: nixiaoming 
---
 fs/notify/fanotify/fanotify.c  | 5 -
 fs/notify/fanotify/fanotify_user.c | 5 +++--
 include/linux/fsnotify_backend.h   | 1 +
 include/uapi/linux/fanotify.h  | 1 +
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 94b5215..5a3af87 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -171,7 +171,10 @@ struct fanotify_event_info *fanotify_alloc_event(struct 
fsnotify_group *group,
goto out;
 init: __maybe_unused
fsnotify_init_event(>fse, inode, mask);
-   event->tgid = get_pid(task_tgid(current));
+   if (group->fanotify_data.should_report_tid)
+   event->tgid = get_pid(task_pid(current));
+   else
+   event->tgid = get_pid(task_tgid(current));
if (path) {
event->path = *path;
path_get(>path);
diff --git a/fs/notify/fanotify/fanotify_user.c 
b/fs/notify/fanotify/fanotify_user.c
index 6905488..3aa425b 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -693,9 +693,9 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, 
unsigned int, event_f_flags)
return -EPERM;
 
 #ifdef CONFIG_AUDITSYSCALL
-   if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_ENABLE_AUDIT))
+   if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_ENABLE_AUDIT | 
FAN_EVENT_INFO_TID))
 #else
-   if (flags & ~FAN_ALL_INIT_FLAGS)
+   if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_EVENT_INFO_TID))
 #endif
return -EINVAL;
 
@@ -731,6 +731,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, 
unsigned int, event_f_flags)
}
 
group->fanotify_data.user = user;
+   group->fanotify_data.should_report_tid = (flags & FAN_EVENT_INFO_TID) ? 
true : false;
atomic_inc(>fanotify_listeners);
group->memcg = get_mem_cgroup_from_mm(current->mm);
 
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index b8f4182..44c659f 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -193,6 +193,7 @@ struct fsnotify_group {
unsigned int max_marks;
struct user_struct *user;
bool audit;
+   bool should_report_tid;
} fanotify_data;
 #endif /* CONFIG_FANOTIFY */
};
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 7424791..4eba41c 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -18,6 +18,7 @@
 
 #define FAN_ONDIR  0x4000  /* event occurred against dir */
 
+#define FAN_EVENT_INFO_TID 0x0200  /* reports the thread id of the 
event trigger */
 #define FAN_EVENT_ON_CHILD 0x0800  /* interested in child events */
 
 /* helper events */
-- 
2.10.1

[PATCH v2] fanotify reports the thread id of the event trigger

[nixiaoming]

In order to identify which thread triggered the event in the
multi-threaded program, add the FAN_EVENT_INFO_TID tag in fanotify_init
to select whether to report the event creator's thread id information.

Signed-off-by: nixiaoming 
---
 fs/notify/fanotify/fanotify.c  | 5 -
 fs/notify/fanotify/fanotify_user.c | 5 +++--
 include/linux/fsnotify_backend.h   | 1 +
 include/uapi/linux/fanotify.h  | 1 +
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 94b5215..5a3af87 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -171,7 +171,10 @@ struct fanotify_event_info *fanotify_alloc_event(struct 
fsnotify_group *group,
goto out;
 init: __maybe_unused
fsnotify_init_event(>fse, inode, mask);
-   event->tgid = get_pid(task_tgid(current));
+   if (group->fanotify_data.should_report_tid)
+   event->tgid = get_pid(task_pid(current));
+   else
+   event->tgid = get_pid(task_tgid(current));
if (path) {
event->path = *path;
path_get(>path);
diff --git a/fs/notify/fanotify/fanotify_user.c 
b/fs/notify/fanotify/fanotify_user.c
index 6905488..3aa425b 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -693,9 +693,9 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, 
unsigned int, event_f_flags)
return -EPERM;
 
 #ifdef CONFIG_AUDITSYSCALL
-   if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_ENABLE_AUDIT))
+   if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_ENABLE_AUDIT | 
FAN_EVENT_INFO_TID))
 #else
-   if (flags & ~FAN_ALL_INIT_FLAGS)
+   if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_EVENT_INFO_TID))
 #endif
return -EINVAL;
 
@@ -731,6 +731,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, 
unsigned int, event_f_flags)
}
 
group->fanotify_data.user = user;
+   group->fanotify_data.should_report_tid = (flags & FAN_EVENT_INFO_TID) ? 
true : false;
atomic_inc(>fanotify_listeners);
group->memcg = get_mem_cgroup_from_mm(current->mm);
 
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index b8f4182..44c659f 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -193,6 +193,7 @@ struct fsnotify_group {
unsigned int max_marks;
struct user_struct *user;
bool audit;
+   bool should_report_tid;
} fanotify_data;
 #endif /* CONFIG_FANOTIFY */
};
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 7424791..4eba41c 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -18,6 +18,7 @@
 
 #define FAN_ONDIR  0x4000  /* event occurred against dir */
 
+#define FAN_EVENT_INFO_TID 0x0200  /* reports the thread id of the 
event trigger */
 #define FAN_EVENT_ON_CHILD 0x0800  /* interested in child events */
 
 /* helper events */
-- 
2.10.1

RE: [PATCH] fanotify support save thread id

[Nixiaoming]

On Mon, Sep 17, 2018 6:54 PM Amir Goldstein  wrote:
>On Mon, Sep 17, 2018 at 1:02 PM nixiaoming  wrote:
>>
>> Added FAN_EVENT_INFO_TID to select the thread id of the event trigger
>>
>
>Maybe "to report the thread id of the task that caused the event".
>Also in commit title, I think "reporting thread id" is more to the point
>than "save thread id".
>
Very good advice, I will update the patch description based on your suggestions.

>
>> Signed-off-by: nixiaoming 
>> ---
>> --- a/include/uapi/linux/fanotify.h
>> +++ b/include/uapi/linux/fanotify.h
>> @@ -18,6 +18,7 @@
>>
>>  #define FAN_ONDIR  0x4000  /* event occurred against 
>> dir */
>>
>> +#define FAN_EVENT_INFO_TID 0x0200  /* event save thread id 
>> replace tgid */
>>  #define FAN_EVENT_ON_CHILD 0x0800  /* interested in child 
>> events */
>>
>
>nixiaoming,
>
>You misunderstood me.
>
>I meant that you should add support for flag FAN_EVENT_INFO_TID
>for the fanotify_init() syscall, not the fanotify_mark() syscall.
>
I mistakenly thought that adding a tag to group->fanotify_data in the 
fanotify_init system call would affect the system ABI. 
local tests found that Module.symvers did not change before and after the 
modification.

According to your opinion, I have re-modified a version of fanotify_init
I will send the patch later.

>See this commit from Steve Grubb for example of adding new opt-in
>behavior to fanotify:
>de8cd83e91bc audit: Record fanotify access control decisions
>
>BTW, Steve, did you ever follow up with a man-pages patch?
>
>Thanks,
>Amir.
>

thanks

RE: [PATCH] fanotify support save thread id

[Nixiaoming]

On Mon, Sep 17, 2018 6:54 PM Amir Goldstein  wrote:
>On Mon, Sep 17, 2018 at 1:02 PM nixiaoming  wrote:
>>
>> Added FAN_EVENT_INFO_TID to select the thread id of the event trigger
>>
>
>Maybe "to report the thread id of the task that caused the event".
>Also in commit title, I think "reporting thread id" is more to the point
>than "save thread id".
>
Very good advice, I will update the patch description based on your suggestions.

>
>> Signed-off-by: nixiaoming 
>> ---
>> --- a/include/uapi/linux/fanotify.h
>> +++ b/include/uapi/linux/fanotify.h
>> @@ -18,6 +18,7 @@
>>
>>  #define FAN_ONDIR  0x4000  /* event occurred against 
>> dir */
>>
>> +#define FAN_EVENT_INFO_TID 0x0200  /* event save thread id 
>> replace tgid */
>>  #define FAN_EVENT_ON_CHILD 0x0800  /* interested in child 
>> events */
>>
>
>nixiaoming,
>
>You misunderstood me.
>
>I meant that you should add support for flag FAN_EVENT_INFO_TID
>for the fanotify_init() syscall, not the fanotify_mark() syscall.
>
I mistakenly thought that adding a tag to group->fanotify_data in the 
fanotify_init system call would affect the system ABI. 
local tests found that Module.symvers did not change before and after the 
modification.

According to your opinion, I have re-modified a version of fanotify_init
I will send the patch later.

>See this commit from Steve Grubb for example of adding new opt-in
>behavior to fanotify:
>de8cd83e91bc audit: Record fanotify access control decisions
>
>BTW, Steve, did you ever follow up with a man-pages patch?
>
>Thanks,
>Amir.
>

thanks

//RE: [PATCH] fix memory leak in ramoops_init

[Nixiaoming]

I am very sorry, I sent the wrong email, please ignore the patch just now.

-Original Message-
From: Nixiaoming 
Sent: Monday, September 17, 2018 5:16 PM
To: j...@suse.cz; amir7...@gmail.com
Cc: Nixiaoming ; linux-fsde...@vger.kernel.org; 
linux-kernel@vger.kernel.org
Subject: [PATCH] fix memory leak in ramoops_init

1, memory leak in ramoops_register_dummy.
   dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
   but no free when platform_device_register_data return fail

2, if kzalloc(sizeof(*dummy_data), GFP_KERNEL) return NULL,
but platform_driver_register(_driver) return 0
   kfree(NULL) in ramoops_exit
so, add return val for ramoops_register_dummy, and check it in ramoops_init

3, memory leak in ramoops_init.
   miss platform_device_unregister(dummy) and kfree(dummy_data)
   when platform_driver_register(_driver) return fail

Signed-off-by: nixiaoming 
---
 fs/pstore/ram.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index bd9812e..331b600 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -604,17 +604,17 @@ static struct platform_driver ramoops_driver = {
},
 };
 
-static void ramoops_register_dummy(void)
+static int ramoops_register_dummy(void)
 {
if (!mem_size)
-   return;
+   return -EINVAL;
 
pr_info("using module parameters\n");
 
dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
if (!dummy_data) {
pr_info("could not allocate pdata\n");
-   return;
+   return -ENOMEM;
}
 
dummy_data->mem_size = mem_size;
@@ -636,13 +636,25 @@ static void ramoops_register_dummy(void)
if (IS_ERR(dummy)) {
pr_info("could not create platform device: %ld\n",
PTR_ERR(dummy));
+   kfree(dummy_data);
+   return PTR_ERR(dummy);
}
+   return 0;
 }
 
 static int __init ramoops_init(void)
 {
-   ramoops_register_dummy();
-   return platform_driver_register(_driver);
+   int ret = ramoops_register_dummy();
+
+   if (ret != 0)
+   return ret;
+
+   ret = platform_driver_register(_driver);
+   if (ret != 0) {
+   platform_device_unregister(dummy);
+   kfree(dummy_data);
+   }
+   return ret;
 }
 postcore_initcall(ramoops_init);
 
-- 
1.9.0

//RE: [PATCH] fix memory leak in ramoops_init

[Nixiaoming]

I am very sorry, I sent the wrong email, please ignore the patch just now.

-Original Message-
From: Nixiaoming 
Sent: Monday, September 17, 2018 5:16 PM
To: j...@suse.cz; amir7...@gmail.com
Cc: Nixiaoming ; linux-fsde...@vger.kernel.org; 
linux-kernel@vger.kernel.org
Subject: [PATCH] fix memory leak in ramoops_init

1, memory leak in ramoops_register_dummy.
   dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
   but no free when platform_device_register_data return fail

2, if kzalloc(sizeof(*dummy_data), GFP_KERNEL) return NULL,
but platform_driver_register(_driver) return 0
   kfree(NULL) in ramoops_exit
so, add return val for ramoops_register_dummy, and check it in ramoops_init

3, memory leak in ramoops_init.
   miss platform_device_unregister(dummy) and kfree(dummy_data)
   when platform_driver_register(_driver) return fail

Signed-off-by: nixiaoming 
---
 fs/pstore/ram.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index bd9812e..331b600 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -604,17 +604,17 @@ static struct platform_driver ramoops_driver = {
},
 };
 
-static void ramoops_register_dummy(void)
+static int ramoops_register_dummy(void)
 {
if (!mem_size)
-   return;
+   return -EINVAL;
 
pr_info("using module parameters\n");
 
dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
if (!dummy_data) {
pr_info("could not allocate pdata\n");
-   return;
+   return -ENOMEM;
}
 
dummy_data->mem_size = mem_size;
@@ -636,13 +636,25 @@ static void ramoops_register_dummy(void)
if (IS_ERR(dummy)) {
pr_info("could not create platform device: %ld\n",
PTR_ERR(dummy));
+   kfree(dummy_data);
+   return PTR_ERR(dummy);
}
+   return 0;
 }
 
 static int __init ramoops_init(void)
 {
-   ramoops_register_dummy();
-   return platform_driver_register(_driver);
+   int ret = ramoops_register_dummy();
+
+   if (ret != 0)
+   return ret;
+
+   ret = platform_driver_register(_driver);
+   if (ret != 0) {
+   platform_device_unregister(dummy);
+   kfree(dummy_data);
+   }
+   return ret;
 }
 postcore_initcall(ramoops_init);
 
-- 
1.9.0

[PATCH] fanotify support save thread id

[nixiaoming]

Added FAN_EVENT_INFO_TID to select the thread id of the event trigger

Signed-off-by: nixiaoming 
---
 fs/notify/fanotify/fanotify.c  | 66 +-
 fs/notify/fanotify/fanotify.h  |  2 +-
 fs/notify/fanotify/fanotify_user.c |  4 +--
 include/uapi/linux/fanotify.h  |  1 +
 4 files changed, 48 insertions(+), 25 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 94b5215..cf6ff4e 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -89,14 +89,36 @@ static int fanotify_get_response(struct fsnotify_group 
*group,
return ret;
 }
 
+static void fanotify_iter_mask(struct fsnotify_iter_info *iter_info,
+ u32 event_mask, __u32 *marks_mask, __u32 *marks_ignored_mask)
+{
+   int type;
+   struct fsnotify_mark *mark;
+
+   fsnotify_foreach_obj_type(type) {
+   if (!fsnotify_iter_should_report_type(iter_info, type))
+   continue;
+   mark = iter_info->marks[type];
+   /*
+* if the event is for a child and this inode doesn't care about
+* events on the child, don't send it!
+*/
+   if (type == FSNOTIFY_OBJ_TYPE_INODE &&
+   (event_mask & FS_EVENT_ON_CHILD) &&
+   !(mark->mask & FS_EVENT_ON_CHILD))
+   continue;
+
+   *marks_mask |= mark->mask;
+   *marks_ignored_mask |= mark->ignored_mask;
+   }
+}
+
 static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
   u32 event_mask, const void *data,
   int data_type)
 {
__u32 marks_mask = 0, marks_ignored_mask = 0;
const struct path *path = data;
-   struct fsnotify_mark *mark;
-   int type;
 
pr_debug("%s: report_mask=%x mask=%x data=%p data_type=%d\n",
 __func__, iter_info->report_mask, event_mask, data, data_type);
@@ -110,23 +132,7 @@ static bool fanotify_should_send_event(struct 
fsnotify_iter_info *iter_info,
!d_can_lookup(path->dentry))
return false;
 
-   fsnotify_foreach_obj_type(type) {
-   if (!fsnotify_iter_should_report_type(iter_info, type))
-   continue;
-   mark = iter_info->marks[type];
-   /*
-* if the event is for a child and this inode doesn't care about
-* events on the child, don't send it!
-*/
-   if (type == FSNOTIFY_OBJ_TYPE_INODE &&
-   (event_mask & FS_EVENT_ON_CHILD) &&
-   !(mark->mask & FS_EVENT_ON_CHILD))
-   continue;
-
-   marks_mask |= mark->mask;
-   marks_ignored_mask |= mark->ignored_mask;
-   }
-
+   fanotify_iter_mask(iter_info, event_mask, _mask, 
_ignored_mask);
if (d_is_dir(path->dentry) &&
!(marks_mask & FS_ISDIR & ~marks_ignored_mask))
return false;
@@ -138,9 +144,20 @@ static bool fanotify_should_send_event(struct 
fsnotify_iter_info *iter_info,
return false;
 }
 
+static bool fanotify_should_save_tid(struct fsnotify_iter_info *iter_info,
+u32 event_mask)
+{
+   __u32 marks_mask = 0, marks_ignored_mask = 0;
+
+   fanotify_iter_mask(iter_info, event_mask, _mask, 
_ignored_mask);
+   if (marks_mask & FAN_EVENT_INFO_TID)
+   return true;
+   return false;
+}
+
 struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
 struct inode *inode, u32 mask,
-const struct path *path)
+const struct path *path, bool 
should_save_tid)
 {
struct fanotify_event_info *event = NULL;
gfp_t gfp = GFP_KERNEL_ACCOUNT;
@@ -171,7 +188,10 @@ struct fanotify_event_info *fanotify_alloc_event(struct 
fsnotify_group *group,
goto out;
 init: __maybe_unused
fsnotify_init_event(>fse, inode, mask);
-   event->tgid = get_pid(task_tgid(current));
+   if (should_save_tid)
+   event->tgid = get_pid(task_pid(current));
+   else
+   event->tgid = get_pid(task_tgid(current));
if (path) {
event->path = *path;
path_get(>path);
@@ -193,6 +213,7 @@ static int fanotify_handle_event(struct fsnotify_group 
*group,
int ret = 0;
struct fanotify_event_info *event;
struct fsnotify_event *fsn_event;
+   bool should_save_tid;
 
BUILD_BUG_ON(FAN_ACCESS != FS_ACCESS);
BUILD_BUG_ON(FAN_MODIFY != FS_MODIFY);
@@ -219,8 +240,9 @@ static int fanotify_handl

[PATCH] fanotify support save thread id

[nixiaoming]

Added FAN_EVENT_INFO_TID to select the thread id of the event trigger

Signed-off-by: nixiaoming 
---
 fs/notify/fanotify/fanotify.c  | 66 +-
 fs/notify/fanotify/fanotify.h  |  2 +-
 fs/notify/fanotify/fanotify_user.c |  4 +--
 include/uapi/linux/fanotify.h  |  1 +
 4 files changed, 48 insertions(+), 25 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 94b5215..cf6ff4e 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -89,14 +89,36 @@ static int fanotify_get_response(struct fsnotify_group 
*group,
return ret;
 }
 
+static void fanotify_iter_mask(struct fsnotify_iter_info *iter_info,
+ u32 event_mask, __u32 *marks_mask, __u32 *marks_ignored_mask)
+{
+   int type;
+   struct fsnotify_mark *mark;
+
+   fsnotify_foreach_obj_type(type) {
+   if (!fsnotify_iter_should_report_type(iter_info, type))
+   continue;
+   mark = iter_info->marks[type];
+   /*
+* if the event is for a child and this inode doesn't care about
+* events on the child, don't send it!
+*/
+   if (type == FSNOTIFY_OBJ_TYPE_INODE &&
+   (event_mask & FS_EVENT_ON_CHILD) &&
+   !(mark->mask & FS_EVENT_ON_CHILD))
+   continue;
+
+   *marks_mask |= mark->mask;
+   *marks_ignored_mask |= mark->ignored_mask;
+   }
+}
+
 static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
   u32 event_mask, const void *data,
   int data_type)
 {
__u32 marks_mask = 0, marks_ignored_mask = 0;
const struct path *path = data;
-   struct fsnotify_mark *mark;
-   int type;
 
pr_debug("%s: report_mask=%x mask=%x data=%p data_type=%d\n",
 __func__, iter_info->report_mask, event_mask, data, data_type);
@@ -110,23 +132,7 @@ static bool fanotify_should_send_event(struct 
fsnotify_iter_info *iter_info,
!d_can_lookup(path->dentry))
return false;
 
-   fsnotify_foreach_obj_type(type) {
-   if (!fsnotify_iter_should_report_type(iter_info, type))
-   continue;
-   mark = iter_info->marks[type];
-   /*
-* if the event is for a child and this inode doesn't care about
-* events on the child, don't send it!
-*/
-   if (type == FSNOTIFY_OBJ_TYPE_INODE &&
-   (event_mask & FS_EVENT_ON_CHILD) &&
-   !(mark->mask & FS_EVENT_ON_CHILD))
-   continue;
-
-   marks_mask |= mark->mask;
-   marks_ignored_mask |= mark->ignored_mask;
-   }
-
+   fanotify_iter_mask(iter_info, event_mask, _mask, 
_ignored_mask);
if (d_is_dir(path->dentry) &&
!(marks_mask & FS_ISDIR & ~marks_ignored_mask))
return false;
@@ -138,9 +144,20 @@ static bool fanotify_should_send_event(struct 
fsnotify_iter_info *iter_info,
return false;
 }
 
+static bool fanotify_should_save_tid(struct fsnotify_iter_info *iter_info,
+u32 event_mask)
+{
+   __u32 marks_mask = 0, marks_ignored_mask = 0;
+
+   fanotify_iter_mask(iter_info, event_mask, _mask, 
_ignored_mask);
+   if (marks_mask & FAN_EVENT_INFO_TID)
+   return true;
+   return false;
+}
+
 struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
 struct inode *inode, u32 mask,
-const struct path *path)
+const struct path *path, bool 
should_save_tid)
 {
struct fanotify_event_info *event = NULL;
gfp_t gfp = GFP_KERNEL_ACCOUNT;
@@ -171,7 +188,10 @@ struct fanotify_event_info *fanotify_alloc_event(struct 
fsnotify_group *group,
goto out;
 init: __maybe_unused
fsnotify_init_event(>fse, inode, mask);
-   event->tgid = get_pid(task_tgid(current));
+   if (should_save_tid)
+   event->tgid = get_pid(task_pid(current));
+   else
+   event->tgid = get_pid(task_tgid(current));
if (path) {
event->path = *path;
path_get(>path);
@@ -193,6 +213,7 @@ static int fanotify_handle_event(struct fsnotify_group 
*group,
int ret = 0;
struct fanotify_event_info *event;
struct fsnotify_event *fsn_event;
+   bool should_save_tid;
 
BUILD_BUG_ON(FAN_ACCESS != FS_ACCESS);
BUILD_BUG_ON(FAN_MODIFY != FS_MODIFY);
@@ -219,8 +240,9 @@ static int fanotify_handl

[PATCH] fix memory leak in ramoops_init

[nixiaoming]

1, memory leak in ramoops_register_dummy.
   dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
   but no free when platform_device_register_data return fail

2, if kzalloc(sizeof(*dummy_data), GFP_KERNEL) return NULL,
but platform_driver_register(_driver) return 0
   kfree(NULL) in ramoops_exit
so, add return val for ramoops_register_dummy, and check it in ramoops_init

3, memory leak in ramoops_init.
   miss platform_device_unregister(dummy) and kfree(dummy_data)
   when platform_driver_register(_driver) return fail

Signed-off-by: nixiaoming 
---
 fs/pstore/ram.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index bd9812e..331b600 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -604,17 +604,17 @@ static struct platform_driver ramoops_driver = {
},
 };
 
-static void ramoops_register_dummy(void)
+static int ramoops_register_dummy(void)
 {
if (!mem_size)
-   return;
+   return -EINVAL;
 
pr_info("using module parameters\n");
 
dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
if (!dummy_data) {
pr_info("could not allocate pdata\n");
-   return;
+   return -ENOMEM;
}
 
dummy_data->mem_size = mem_size;
@@ -636,13 +636,25 @@ static void ramoops_register_dummy(void)
if (IS_ERR(dummy)) {
pr_info("could not create platform device: %ld\n",
PTR_ERR(dummy));
+   kfree(dummy_data);
+   return PTR_ERR(dummy);
}
+   return 0;
 }
 
 static int __init ramoops_init(void)
 {
-   ramoops_register_dummy();
-   return platform_driver_register(_driver);
+   int ret = ramoops_register_dummy();
+
+   if (ret != 0)
+   return ret;
+
+   ret = platform_driver_register(_driver);
+   if (ret != 0) {
+   platform_device_unregister(dummy);
+   kfree(dummy_data);
+   }
+   return ret;
 }
 postcore_initcall(ramoops_init);
 
-- 
1.9.0

[PATCH] fix memory leak in ramoops_init

[nixiaoming]

1, memory leak in ramoops_register_dummy.
   dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
   but no free when platform_device_register_data return fail

2, if kzalloc(sizeof(*dummy_data), GFP_KERNEL) return NULL,
but platform_driver_register(_driver) return 0
   kfree(NULL) in ramoops_exit
so, add return val for ramoops_register_dummy, and check it in ramoops_init

3, memory leak in ramoops_init.
   miss platform_device_unregister(dummy) and kfree(dummy_data)
   when platform_driver_register(_driver) return fail

Signed-off-by: nixiaoming 
---
 fs/pstore/ram.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index bd9812e..331b600 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -604,17 +604,17 @@ static struct platform_driver ramoops_driver = {
},
 };
 
-static void ramoops_register_dummy(void)
+static int ramoops_register_dummy(void)
 {
if (!mem_size)
-   return;
+   return -EINVAL;
 
pr_info("using module parameters\n");
 
dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
if (!dummy_data) {
pr_info("could not allocate pdata\n");
-   return;
+   return -ENOMEM;
}
 
dummy_data->mem_size = mem_size;
@@ -636,13 +636,25 @@ static void ramoops_register_dummy(void)
if (IS_ERR(dummy)) {
pr_info("could not create platform device: %ld\n",
PTR_ERR(dummy));
+   kfree(dummy_data);
+   return PTR_ERR(dummy);
}
+   return 0;
 }
 
 static int __init ramoops_init(void)
 {
-   ramoops_register_dummy();
-   return platform_driver_register(_driver);
+   int ret = ramoops_register_dummy();
+
+   if (ret != 0)
+   return ret;
+
+   ret = platform_driver_register(_driver);
+   if (ret != 0) {
+   platform_device_unregister(dummy);
+   kfree(dummy_data);
+   }
+   return ret;
 }
 postcore_initcall(ramoops_init);
 
-- 
1.9.0

RE: Is it possible to add pid and comm members to the event structure to increase the display of user and thread information?

[Nixiaoming]

on Thu, Sep 13, 2018 at 8:32 PM Amir Goldstein wrote:
>On Thu, Sep 13, 2018 at 2:25 PM Nixiaoming  wrote:
>>
>> On Tue, Sep 11, 2018 at 11:12 PM Amir Goldstein  wrote:
>> >On Tue, Sep 11, 2018 at 9:51 AM Nixiaoming  wrote:
>> >>
>> >> Inotify api cannot display information about users and processes.
>> >> That is, you can only know that the file event is generated, but you 
>> >> don't know who triggered the event, which is not conducive to fault 
>> >> location.
>> >> Is it possible to add pid and comm members to the event structure to 
>> >> increase the display of user and thread information?
>> >>
>> >
>> >"Is it possible?" is not the only relevant question.
>> >I suppose your patch can sort of works, but it exposes information to
>> >potentially unpriveleged
>> >processes, even exposes pid values outside of the process pid namespace.
>> >
>> >While those issues could be addressed, you can't change the format
>> >struct inotify_event
>> >without breaking existing applications.
>> >
>> In order to improve the fault location capability, can we make incompatible 
>> interface changes?
>
>Not unless application/sysadmin/distro opts-in for the incompatible change.
>
>>
>> >I guess you are not using fanotify API, which already provides pid
>> >information (albiet tgid),
>> >because it lacks other functionality that you need? Which
>> >functionality might that be?
>> >Is it directory modification events?
>> >If so than you might be interested in my effort to add support for
>> >those events to fanotify:
>> >https://github.com/amir73il/fsnotify-utils/wiki/Super-block-root-watch
>> >
>> The fanotify API does not support monitoring file deletion events
>
>Yes, I am working toward that goal.
>
>> The fanotify API supports tgid display,
>> but for multi-threaded programs,
>> it still cannot accurately identify which thread triggered the event.
>> Can I modify tgid to pid?
>> - event->tgid = get_pid(task_tgid(current));
>> + event->tgid = get_pid(task_pid(current));
>>
>
>So if you would like to change that you need to add a new flag to
>fanotify_init (e.g. FAN_EVENT_INFO_TID)
>new applications that would opt-in for the flag will get task_pid()
>while existing application will keep getting task_tgid()
>new applications will get -EINVAL when passing FAN_EVENT_INFO_TID
>to fanotify_init() on an old kernel and they could then fall back to getting
>tgid in events and be aware of that fact.
>
>Thanks,
>Amir.
>

Thank you for your guidance
I am modifying the code test locally,
The kernel mode selects whether to display the thread id information 
according to the FAN_EVENT_INFO_TID tag when fanotify_alloc_event is called 
the user mode test program adds FAN_EVENT_INFO_TID to the fanotify_markd 
parameter mask,  
now, it can accurately display which thread triggered the event.

I will send the patch later, please help me to review it

thanks

RE: Is it possible to add pid and comm members to the event structure to increase the display of user and thread information?

[Nixiaoming]

on Thu, Sep 13, 2018 at 8:32 PM Amir Goldstein wrote:
>On Thu, Sep 13, 2018 at 2:25 PM Nixiaoming  wrote:
>>
>> On Tue, Sep 11, 2018 at 11:12 PM Amir Goldstein  wrote:
>> >On Tue, Sep 11, 2018 at 9:51 AM Nixiaoming  wrote:
>> >>
>> >> Inotify api cannot display information about users and processes.
>> >> That is, you can only know that the file event is generated, but you 
>> >> don't know who triggered the event, which is not conducive to fault 
>> >> location.
>> >> Is it possible to add pid and comm members to the event structure to 
>> >> increase the display of user and thread information?
>> >>
>> >
>> >"Is it possible?" is not the only relevant question.
>> >I suppose your patch can sort of works, but it exposes information to
>> >potentially unpriveleged
>> >processes, even exposes pid values outside of the process pid namespace.
>> >
>> >While those issues could be addressed, you can't change the format
>> >struct inotify_event
>> >without breaking existing applications.
>> >
>> In order to improve the fault location capability, can we make incompatible 
>> interface changes?
>
>Not unless application/sysadmin/distro opts-in for the incompatible change.
>
>>
>> >I guess you are not using fanotify API, which already provides pid
>> >information (albiet tgid),
>> >because it lacks other functionality that you need? Which
>> >functionality might that be?
>> >Is it directory modification events?
>> >If so than you might be interested in my effort to add support for
>> >those events to fanotify:
>> >https://github.com/amir73il/fsnotify-utils/wiki/Super-block-root-watch
>> >
>> The fanotify API does not support monitoring file deletion events
>
>Yes, I am working toward that goal.
>
>> The fanotify API supports tgid display,
>> but for multi-threaded programs,
>> it still cannot accurately identify which thread triggered the event.
>> Can I modify tgid to pid?
>> - event->tgid = get_pid(task_tgid(current));
>> + event->tgid = get_pid(task_pid(current));
>>
>
>So if you would like to change that you need to add a new flag to
>fanotify_init (e.g. FAN_EVENT_INFO_TID)
>new applications that would opt-in for the flag will get task_pid()
>while existing application will keep getting task_tgid()
>new applications will get -EINVAL when passing FAN_EVENT_INFO_TID
>to fanotify_init() on an old kernel and they could then fall back to getting
>tgid in events and be aware of that fact.
>
>Thanks,
>Amir.
>

Thank you for your guidance
I am modifying the code test locally,
The kernel mode selects whether to display the thread id information 
according to the FAN_EVENT_INFO_TID tag when fanotify_alloc_event is called 
the user mode test program adds FAN_EVENT_INFO_TID to the fanotify_markd 
parameter mask,  
now, it can accurately display which thread triggered the event.

I will send the patch later, please help me to review it

thanks

RE: Is it possible to add pid and comm members to the event structure to increase the display of user and thread information?

[Nixiaoming]

On Tue, Sep 11, 2018 at 11:12 PM Amir Goldstein  wrote:
>On Tue, Sep 11, 2018 at 9:51 AM Nixiaoming  wrote:
>>
>> Inotify api cannot display information about users and processes.
>> That is, you can only know that the file event is generated, but you don't 
>> know who triggered the event, which is not conducive to fault location.
>> Is it possible to add pid and comm members to the event structure to 
>> increase the display of user and thread information?
>>
>
>"Is it possible?" is not the only relevant question.
>I suppose your patch can sort of works, but it exposes information to
>potentially unpriveleged
>processes, even exposes pid values outside of the process pid namespace.
>
>While those issues could be addressed, you can't change the format
>struct inotify_event
>without breaking existing applications.
>
In order to improve the fault location capability, can we make incompatible 
interface changes?

>I guess you are not using fanotify API, which already provides pid
>information (albiet tgid),
>because it lacks other functionality that you need? Which
>functionality might that be?
>Is it directory modification events?
>If so than you might be interested in my effort to add support for
>those events to fanotify:
>https://github.com/amir73il/fsnotify-utils/wiki/Super-block-root-watch
>
The fanotify API does not support monitoring file deletion events
The fanotify API supports tgid display,
but for multi-threaded programs,
it still cannot accurately identify which thread triggered the event.
Can I modify tgid to pid?
- event->tgid = get_pid(task_tgid(current));
+ event->tgid = get_pid(task_pid(current));

>Your support, should you choose to offer it, could be in the form of
>testing patches
>and/or just by putting forward your use case as an example for the
>need of an extended
>fanotify API.
>
>Thanks,
>Amir.
>

Thanks

RE: Is it possible to add pid and comm members to the event structure to increase the display of user and thread information?

[Nixiaoming]

On Tue, Sep 11, 2018 at 11:12 PM Amir Goldstein  wrote:
>On Tue, Sep 11, 2018 at 9:51 AM Nixiaoming  wrote:
>>
>> Inotify api cannot display information about users and processes.
>> That is, you can only know that the file event is generated, but you don't 
>> know who triggered the event, which is not conducive to fault location.
>> Is it possible to add pid and comm members to the event structure to 
>> increase the display of user and thread information?
>>
>
>"Is it possible?" is not the only relevant question.
>I suppose your patch can sort of works, but it exposes information to
>potentially unpriveleged
>processes, even exposes pid values outside of the process pid namespace.
>
>While those issues could be addressed, you can't change the format
>struct inotify_event
>without breaking existing applications.
>
In order to improve the fault location capability, can we make incompatible 
interface changes?

>I guess you are not using fanotify API, which already provides pid
>information (albiet tgid),
>because it lacks other functionality that you need? Which
>functionality might that be?
>Is it directory modification events?
>If so than you might be interested in my effort to add support for
>those events to fanotify:
>https://github.com/amir73il/fsnotify-utils/wiki/Super-block-root-watch
>
The fanotify API does not support monitoring file deletion events
The fanotify API supports tgid display,
but for multi-threaded programs,
it still cannot accurately identify which thread triggered the event.
Can I modify tgid to pid?
- event->tgid = get_pid(task_tgid(current));
+ event->tgid = get_pid(task_pid(current));

>Your support, should you choose to offer it, could be in the form of
>testing patches
>and/or just by putting forward your use case as an example for the
>need of an extended
>fanotify API.
>
>Thanks,
>Amir.
>

Thanks

Is it possible to add pid and comm members to the event structure to increase the display of user and thread information?

[Nixiaoming]

Inotify api cannot display information about users and processes.
That is, you can only know that the file event is generated, but you don't know 
who triggered the event, which is not conducive to fault location.
Is it possible to add pid and comm members to the event structure to increase 
the display of user and thread information?

Example:
diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h
index 7e4578d..be91844 100644
--- a/fs/notify/inotify/inotify.h
+++ b/fs/notify/inotify/inotify.h
@@ -7,6 +7,8 @@ struct inotify_event_info {
struct fsnotify_event fse;
int wd;
u32 sync_cookie;
+ int pid;
+ char comm[TASK_COMM_LEN];
int name_len;
char name[];
 };
diff --git a/fs/notify/inotify/inotify_fsnotify.c 
b/fs/notify/inotify/inotify_fsnotify.c
index f4184b4..f7ad298 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -117,6 +117,8 @@ int inotify_handle_event(struct fsnotify_group *group,
fsnotify_init_event(fsn_event, inode, mask);
event->wd = i_mark->wd;
event->sync_cookie = cookie;
+ event->pid = current->pid;
+ strncpy(event->comm, current->comm, TASK_COMM_LEN);
event->name_len = len;
if (len)
strcpy(event->name, file_name);

Is it possible to add pid and comm members to the event structure to increase the display of user and thread information?

[Nixiaoming]

Inotify api cannot display information about users and processes.
That is, you can only know that the file event is generated, but you don't know 
who triggered the event, which is not conducive to fault location.
Is it possible to add pid and comm members to the event structure to increase 
the display of user and thread information?

Example:
diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h
index 7e4578d..be91844 100644
--- a/fs/notify/inotify/inotify.h
+++ b/fs/notify/inotify/inotify.h
@@ -7,6 +7,8 @@ struct inotify_event_info {
struct fsnotify_event fse;
int wd;
u32 sync_cookie;
+ int pid;
+ char comm[TASK_COMM_LEN];
int name_len;
char name[];
 };
diff --git a/fs/notify/inotify/inotify_fsnotify.c 
b/fs/notify/inotify/inotify_fsnotify.c
index f4184b4..f7ad298 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -117,6 +117,8 @@ int inotify_handle_event(struct fsnotify_group *group,
fsnotify_init_event(fsn_event, inode, mask);
event->wd = i_mark->wd;
event->sync_cookie = cookie;
+ event->pid = current->pid;
+ strncpy(event->comm, current->comm, TASK_COMM_LEN);
event->name_len = len;
if (len)
strcpy(event->name, file_name);

[PATCH v3] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id

[nixiaoming]

READ_BUF(8);
dummy = be32_to_cpup(p++);
dummy = be32_to_cpup(p++);
...
READ_BUF(4);
dummy = be32_to_cpup(p++);

Assigning value to "dummy" here, but that stored value
is overwritten before it can be used.
At the same time READ_BUF() will re-update the pointer p.

delete invalid assignment statements

Signed-off-by: nixiaoming 
Signed-off-by: Chuck Lever 
Signed-off-by: Trond Myklebust 
Signed-off-by: J. Bruce Fields 
---
 fs/nfsd/nfs4xdr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index a96843c..e669e20 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1390,10 +1390,8 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp,
p += XDR_QUADLEN(dummy);
}
 
-   /* ssp_window and ssp_num_gss_handles */
+   /* ignore ssp_window and ssp_num_gss_handles */
READ_BUF(8);
-   dummy = be32_to_cpup(p++);
-   dummy = be32_to_cpup(p++);
break;
default:
goto xdr_error;
-- 
2.10.1

[PATCH v3] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id

[nixiaoming]

READ_BUF(8);
dummy = be32_to_cpup(p++);
dummy = be32_to_cpup(p++);
...
READ_BUF(4);
dummy = be32_to_cpup(p++);

Assigning value to "dummy" here, but that stored value
is overwritten before it can be used.
At the same time READ_BUF() will re-update the pointer p.

delete invalid assignment statements

Signed-off-by: nixiaoming 
Signed-off-by: Chuck Lever 
Signed-off-by: Trond Myklebust 
Signed-off-by: J. Bruce Fields 
---
 fs/nfsd/nfs4xdr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index a96843c..e669e20 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1390,10 +1390,8 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp,
p += XDR_QUADLEN(dummy);
}
 
-   /* ssp_window and ssp_num_gss_handles */
+   /* ignore ssp_window and ssp_num_gss_handles */
READ_BUF(8);
-   dummy = be32_to_cpup(p++);
-   dummy = be32_to_cpup(p++);
break;
default:
goto xdr_error;
-- 
2.10.1

RE: [PATCH v2] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id

[Nixiaoming]

On Wednesday, August 01, 2018 11:18 PM ,  J. Bruce Fields  wrote: 
>On Mon, Jul 23, 2018 at 09:57:11AM +0800, nixiaoming wrote:
>> READ_BUF(8);
>> dummy = be32_to_cpup(p++);
>> dummy = be32_to_cpup(p++);
>> ...
>> READ_BUF(4);
>> dummy = be32_to_cpup(p++);
>> 
>> Assigning value to "dummy" here, but that stored value
>> is overwritten before it can be used.
>> At the same time READ_BUF() will re-update the pointer p.
>> 
>> delete invalid assignment statements
>
>Thanks, applying with a minor comment tweak to clarify that we're
>intentionally not reading these:
>
>-  /* ssp_window and ssp_num_gss_handles */
>+  /* ignore ssp_window and ssp_num_gss_handles: */
>   READ_BUF(8);
>   break;
>
>--b.
>

Thanks for your advice
I will update the patch as soon as possible according to your advice.

>> 
>> Signed-off-by: nixiaoming 
>> Signed-off-by: Chuck Lever 2
>> Signed-off-by: Trond Myklebust 
>> ---
>>  fs/nfsd/nfs4xdr.c | 2 --
>>  1 file changed, 2 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index a96843c..375ad4b 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -1392,8 +1392,6 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs 
>> *argp,
>>  
>>  /* ssp_window and ssp_num_gss_handles */
>>  READ_BUF(8);
>> -dummy = be32_to_cpup(p++);
>> -dummy = be32_to_cpup(p++);
>>  break;
>>  default:
>>  goto xdr_error;
>> -- 
>> 2.10.1

RE: [PATCH v2] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id

[Nixiaoming]

On Wednesday, August 01, 2018 11:18 PM ,  J. Bruce Fields  wrote: 
>On Mon, Jul 23, 2018 at 09:57:11AM +0800, nixiaoming wrote:
>> READ_BUF(8);
>> dummy = be32_to_cpup(p++);
>> dummy = be32_to_cpup(p++);
>> ...
>> READ_BUF(4);
>> dummy = be32_to_cpup(p++);
>> 
>> Assigning value to "dummy" here, but that stored value
>> is overwritten before it can be used.
>> At the same time READ_BUF() will re-update the pointer p.
>> 
>> delete invalid assignment statements
>
>Thanks, applying with a minor comment tweak to clarify that we're
>intentionally not reading these:
>
>-  /* ssp_window and ssp_num_gss_handles */
>+  /* ignore ssp_window and ssp_num_gss_handles: */
>   READ_BUF(8);
>   break;
>
>--b.
>

Thanks for your advice
I will update the patch as soon as possible according to your advice.

>> 
>> Signed-off-by: nixiaoming 
>> Signed-off-by: Chuck Lever 2
>> Signed-off-by: Trond Myklebust 
>> ---
>>  fs/nfsd/nfs4xdr.c | 2 --
>>  1 file changed, 2 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index a96843c..375ad4b 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -1392,8 +1392,6 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs 
>> *argp,
>>  
>>  /* ssp_window and ssp_num_gss_handles */
>>  READ_BUF(8);
>> -dummy = be32_to_cpup(p++);
>> -dummy = be32_to_cpup(p++);
>>  break;
>>  default:
>>  goto xdr_error;
>> -- 
>> 2.10.1

maybe resource leak in security/selinux/selinuxfs.c

[Nixiaoming]

advisory:
1 After creating dentry in d_alloc_name, should I call dput to release 
resources before the exception exit?
2 After calling the new_inode to create an inode, should the inode resource be 
released before the exception exit?

If the dentry and inode resources need to be actively released, there are 
multiple resource leaks in security/selinux/selinuxfs.c.

Example:
Linux master branch v4.18-rc5
The function sel_make_avc_files in security/selinux/selinuxfs.c.

1566 static int sel_make_avc_files(struct dentry *dir)
...
1580 for (i = 0; i < ARRAY_SIZE(files); i++) {
1581 struct inode *inode;
1582 struct dentry *dentry;
1583
1584 dentry = d_alloc_name(dir, files[i].name);
1585 if (!dentry)
/*Resource leak: when i!=0, the release action of dentry and inode resources is 
missing*/
1586 return -ENOMEM;
1587
1588 inode = sel_make_inode(dir->d_sb, S_IFREG|files[i].mode);
1589 if (!inode)
/*Resource leak: missing dput(dentry)*/
/*Resource leak: when i!=0, the release action of dentry and inode resources is 
missing*/
1590 return -ENOMEM;
1591
1592 inode->i_fop = files[i].ops;
1593 inode->i_ino = ++fsi->last_ino;
1594 d_add(dentry, inode);
1595 }
1596
1597 return 0;
1598 }

There are similar resource leaking functions:
Sel_make_bools
Sel_make_avc_files
Sel_make_initcon_files
Sel_make_perm_files
Sel_make_class_dir_entries
Sel_make_policycap
Sel_fill_super
Sel_make_policy_nodes
Sel_make_classes

maybe resource leak in security/selinux/selinuxfs.c

[Nixiaoming]

advisory:
1 After creating dentry in d_alloc_name, should I call dput to release 
resources before the exception exit?
2 After calling the new_inode to create an inode, should the inode resource be 
released before the exception exit?

If the dentry and inode resources need to be actively released, there are 
multiple resource leaks in security/selinux/selinuxfs.c.

Example:
Linux master branch v4.18-rc5
The function sel_make_avc_files in security/selinux/selinuxfs.c.

1566 static int sel_make_avc_files(struct dentry *dir)
...
1580 for (i = 0; i < ARRAY_SIZE(files); i++) {
1581 struct inode *inode;
1582 struct dentry *dentry;
1583
1584 dentry = d_alloc_name(dir, files[i].name);
1585 if (!dentry)
/*Resource leak: when i!=0, the release action of dentry and inode resources is 
missing*/
1586 return -ENOMEM;
1587
1588 inode = sel_make_inode(dir->d_sb, S_IFREG|files[i].mode);
1589 if (!inode)
/*Resource leak: missing dput(dentry)*/
/*Resource leak: when i!=0, the release action of dentry and inode resources is 
missing*/
1590 return -ENOMEM;
1591
1592 inode->i_fop = files[i].ops;
1593 inode->i_ino = ++fsi->last_ino;
1594 d_add(dentry, inode);
1595 }
1596
1597 return 0;
1598 }

There are similar resource leaking functions:
Sel_make_bools
Sel_make_avc_files
Sel_make_initcon_files
Sel_make_perm_files
Sel_make_class_dir_entries
Sel_make_policycap
Sel_fill_super
Sel_make_policy_nodes
Sel_make_classes

[PATCH v2] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id

[nixiaoming]

READ_BUF(8);
dummy = be32_to_cpup(p++);
dummy = be32_to_cpup(p++);
...
READ_BUF(4);
dummy = be32_to_cpup(p++);

Assigning value to "dummy" here, but that stored value
is overwritten before it can be used.
At the same time READ_BUF() will re-update the pointer p.

delete invalid assignment statements

Signed-off-by: nixiaoming 
Signed-off-by: Chuck Lever 
Signed-off-by: Trond Myklebust 
---
 fs/nfsd/nfs4xdr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index a96843c..375ad4b 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1392,8 +1392,6 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp,
 
/* ssp_window and ssp_num_gss_handles */
READ_BUF(8);
-   dummy = be32_to_cpup(p++);
-   dummy = be32_to_cpup(p++);
break;
default:
goto xdr_error;
-- 
2.10.1

[PATCH v2] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id

[nixiaoming]

READ_BUF(8);
dummy = be32_to_cpup(p++);
dummy = be32_to_cpup(p++);
...
READ_BUF(4);
dummy = be32_to_cpup(p++);

Assigning value to "dummy" here, but that stored value
is overwritten before it can be used.
At the same time READ_BUF() will re-update the pointer p.

delete invalid assignment statements

Signed-off-by: nixiaoming 
Signed-off-by: Chuck Lever 
Signed-off-by: Trond Myklebust 
---
 fs/nfsd/nfs4xdr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index a96843c..375ad4b 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1392,8 +1392,6 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp,
 
/* ssp_window and ssp_num_gss_handles */
READ_BUF(8);
-   dummy = be32_to_cpup(p++);
-   dummy = be32_to_cpup(p++);
break;
default:
goto xdr_error;
-- 
2.10.1

RE: [PATCH] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id

[Nixiaoming]

On Mon, Jul 23, 2018 at 3:08 AM, Trond Myklebust wrote:
>On Sun, 2018-07-22 at 15:01 -0400, Chuck Lever wrote:
>> > On Jul 22, 2018, at 2:33 PM, Trond Myklebust <
>> > tron...@hammerspace.com> wrote:
>> > 
>> > On Sun, 2018-07-22 at 14:12 -0400, Chuck Lever wrote:
>> > > > On Jul 22, 2018, at 4:50 AM, nixiaoming 
>> > > > wrote:
>> > > > 
>> > > > dummy = be32_to_cpup(p++);
>> > > > dummy = be32_to_cpup(p++);
>> > > > Assigning value to "dummy" here, but that stored value
>> > > > is overwritten before it can be used.
>> > > > 
>> > > > delete invalid assignment statements in
>> > > > nfsd4_decode_exchange_id
>> > > > 
>> > > > Signed-off-by: n00202754 
>> > > > ---
>> > > > fs/nfsd/nfs4xdr.c | 4 ++--
>> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
>> > > > 
>> > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> > > > index a96843c..8e78541 100644
>> > > > --- a/fs/nfsd/nfs4xdr.c
>> > > > +++ b/fs/nfsd/nfs4xdr.c
>> > > > @@ -1392,8 +1392,8 @@ nfsd4_decode_exchange_id(struct
>> > > > nfsd4_compoundargs *argp,
>> > > > 
>> > > >/* ssp_window and ssp_num_gss_handles */
>> > > >READ_BUF(8);
>> > > > -  dummy = be32_to_cpup(p++);
>> > > > -  dummy = be32_to_cpup(p++);
>> > > > +  be32_to_cpup(p++);
>> > > > +  be32_to_cpup(p++);
>> > > 
>> > > If these values are not used, what's the point of byte swapping
>> > > them?
>> > > Surely "p += 2;" should be enough.
>> > > 
>> > > 
>> > > >break;
>> > > >default:
>> > > >goto xdr_error;
>> > 
>> > Given that the value of 'p' isn't used either, why not just delete
>> > those two lines altogether?
>> 
>> Sounds OK, READ_BUF is tracking progress through the buffer,
>> and it already updates "p" as a side-effect.
>> 
>> Might there be some nearby instances of open-coded "p" updates
>> that could also be removed, for similar reasons?
>> 
>
>Probably, yes. I believe that we've said before (and Bruce agreed at
>the time) that we should get rid of READ_BUF() in knfsd as it tends to
>obfuscate these assignments to 'p'. Does anyone have any free cycles to
>work on that?
>
>-- 
>Trond Myklebust
>Linux NFS client maintainer, Hammerspace
>trond.mykleb...@hammerspace.com
>
>

Thank you for your review, 
I will update the patch as soon as possible based on your comments.

Thanks

RE: [PATCH] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id

[Nixiaoming]

On Mon, Jul 23, 2018 at 3:08 AM, Trond Myklebust wrote:
>On Sun, 2018-07-22 at 15:01 -0400, Chuck Lever wrote:
>> > On Jul 22, 2018, at 2:33 PM, Trond Myklebust <
>> > tron...@hammerspace.com> wrote:
>> > 
>> > On Sun, 2018-07-22 at 14:12 -0400, Chuck Lever wrote:
>> > > > On Jul 22, 2018, at 4:50 AM, nixiaoming 
>> > > > wrote:
>> > > > 
>> > > > dummy = be32_to_cpup(p++);
>> > > > dummy = be32_to_cpup(p++);
>> > > > Assigning value to "dummy" here, but that stored value
>> > > > is overwritten before it can be used.
>> > > > 
>> > > > delete invalid assignment statements in
>> > > > nfsd4_decode_exchange_id
>> > > > 
>> > > > Signed-off-by: n00202754 
>> > > > ---
>> > > > fs/nfsd/nfs4xdr.c | 4 ++--
>> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
>> > > > 
>> > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> > > > index a96843c..8e78541 100644
>> > > > --- a/fs/nfsd/nfs4xdr.c
>> > > > +++ b/fs/nfsd/nfs4xdr.c
>> > > > @@ -1392,8 +1392,8 @@ nfsd4_decode_exchange_id(struct
>> > > > nfsd4_compoundargs *argp,
>> > > > 
>> > > >/* ssp_window and ssp_num_gss_handles */
>> > > >READ_BUF(8);
>> > > > -  dummy = be32_to_cpup(p++);
>> > > > -  dummy = be32_to_cpup(p++);
>> > > > +  be32_to_cpup(p++);
>> > > > +  be32_to_cpup(p++);
>> > > 
>> > > If these values are not used, what's the point of byte swapping
>> > > them?
>> > > Surely "p += 2;" should be enough.
>> > > 
>> > > 
>> > > >break;
>> > > >default:
>> > > >goto xdr_error;
>> > 
>> > Given that the value of 'p' isn't used either, why not just delete
>> > those two lines altogether?
>> 
>> Sounds OK, READ_BUF is tracking progress through the buffer,
>> and it already updates "p" as a side-effect.
>> 
>> Might there be some nearby instances of open-coded "p" updates
>> that could also be removed, for similar reasons?
>> 
>
>Probably, yes. I believe that we've said before (and Bruce agreed at
>the time) that we should get rid of READ_BUF() in knfsd as it tends to
>obfuscate these assignments to 'p'. Does anyone have any free cycles to
>work on that?
>
>-- 
>Trond Myklebust
>Linux NFS client maintainer, Hammerspace
>trond.mykleb...@hammerspace.com
>
>

Thank you for your review, 
I will update the patch as soon as possible based on your comments.

Thanks

[PATCH] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id

[nixiaoming]

dummy = be32_to_cpup(p++);
dummy = be32_to_cpup(p++);
Assigning value to "dummy" here, but that stored value
is overwritten before it can be used.

delete invalid assignment statements in nfsd4_decode_exchange_id

Signed-off-by: n00202754 
---
 fs/nfsd/nfs4xdr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index a96843c..8e78541 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1392,8 +1392,8 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp,
 
/* ssp_window and ssp_num_gss_handles */
READ_BUF(8);
-   dummy = be32_to_cpup(p++);
-   dummy = be32_to_cpup(p++);
+   be32_to_cpup(p++);
+   be32_to_cpup(p++);
break;
default:
goto xdr_error;
-- 
2.10.1

[PATCH] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id

[nixiaoming]

dummy = be32_to_cpup(p++);
dummy = be32_to_cpup(p++);
Assigning value to "dummy" here, but that stored value
is overwritten before it can be used.

delete invalid assignment statements in nfsd4_decode_exchange_id

Signed-off-by: n00202754 
---
 fs/nfsd/nfs4xdr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index a96843c..8e78541 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1392,8 +1392,8 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp,
 
/* ssp_window and ssp_num_gss_handles */
READ_BUF(8);
-   dummy = be32_to_cpup(p++);
-   dummy = be32_to_cpup(p++);
+   be32_to_cpup(p++);
+   be32_to_cpup(p++);
break;
default:
goto xdr_error;
-- 
2.10.1

[PATCH] Delete invalid assignment statements in do_sendfile

[nixiaoming]

Assigning value -EINVAL to "retval" here, but that stored value is
overwritten before it can be used.

retval = -EINVAL;

retval = rw_verify_area(WRITE, out.file, _pos, count);

value_overwrite: Overwriting previous write to "retval" with value
from rw_verify_area

delete invalid assignment statements

Signed-off-by: n00202754 
---
 fs/read_write.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 153f8f6..9ab202c 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1407,7 +1407,6 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t 
*ppos,
goto fput_in;
if (!(out.file->f_mode & FMODE_WRITE))
goto fput_out;
-   retval = -EINVAL;
in_inode = file_inode(in.file);
out_inode = file_inode(out.file);
out_pos = out.file->f_pos;
-- 
2.10.1

[PATCH] Delete invalid assignment statements in do_sendfile

[nixiaoming]

Assigning value -EINVAL to "retval" here, but that stored value is
overwritten before it can be used.

retval = -EINVAL;

retval = rw_verify_area(WRITE, out.file, _pos, count);

value_overwrite: Overwriting previous write to "retval" with value
from rw_verify_area

delete invalid assignment statements

Signed-off-by: n00202754 
---
 fs/read_write.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 153f8f6..9ab202c 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1407,7 +1407,6 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t 
*ppos,
goto fput_in;
if (!(out.file->f_mode & FMODE_WRITE))
goto fput_out;
-   retval = -EINVAL;
in_inode = file_inode(in.file);
out_inode = file_inode(out.file);
out_pos = out.file->f_pos;
-- 
2.10.1

RE: [PATCH] mm: Add conditions to avoid out-of-bounds

[Nixiaoming]

I'm very sorry. It was my mistake. 
it won't cross the border here.

Thanks 


-Original Message-
From: Michal Hocko [mailto:mho...@kernel.org] 
Sent: Monday, June 04, 2018 7:20 PM
To: Nixiaoming 
Cc: a...@linux-foundation.org; vdavydov@gmail.com; han...@cmpxchg.org; 
garsi...@embeddedor.com; ktk...@virtuozzo.com; stumm...@codeaurora.org; 
linux-kernel@vger.kernel.org; linux...@kvack.org
Subject: Re: [PATCH] mm: Add conditions to avoid out-of-bounds

On Mon 04-06-18 18:37:35, nixiaoming wrote:
> In the function memcg_init_list_lru
> if call goto fail when i == 0, will cause out-of-bounds at lru->node[i]

How? All I can see is that the fail path does
for (i = i - 1; i >= 0; i--)

so it will not do anything for i=0.
-- 
Michal Hocko
SUSE Labs

RE: [PATCH] mm: Add conditions to avoid out-of-bounds

[Nixiaoming]

I'm very sorry. It was my mistake. 
it won't cross the border here.

Thanks 


-Original Message-
From: Michal Hocko [mailto:mho...@kernel.org] 
Sent: Monday, June 04, 2018 7:20 PM
To: Nixiaoming 
Cc: a...@linux-foundation.org; vdavydov@gmail.com; han...@cmpxchg.org; 
garsi...@embeddedor.com; ktk...@virtuozzo.com; stumm...@codeaurora.org; 
linux-kernel@vger.kernel.org; linux...@kvack.org
Subject: Re: [PATCH] mm: Add conditions to avoid out-of-bounds

On Mon 04-06-18 18:37:35, nixiaoming wrote:
> In the function memcg_init_list_lru
> if call goto fail when i == 0, will cause out-of-bounds at lru->node[i]

How? All I can see is that the fail path does
for (i = i - 1; i >= 0; i--)

so it will not do anything for i=0.
-- 
Michal Hocko
SUSE Labs

[PATCH] mm: Add conditions to avoid out-of-bounds

[nixiaoming]

In the function memcg_init_list_lru
if call goto fail when i == 0, will cause out-of-bounds at lru->node[i]

The same out-of-bounds access scenario exists in the functions
memcg_update_list_lru and __memcg_init_list_lru_node

Signed-off-by: nixiaoming 
---
 mm/list_lru.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/mm/list_lru.c b/mm/list_lru.c
index fcfb6c8..ec6bdd9 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -298,6 +298,9 @@ static void __memcg_destroy_list_lru_node(struct 
list_lru_memcg *memcg_lrus,
 {
int i;
 
+   if (unlikely(begin >= end))
+   return;
+
for (i = begin; i < end; i++)
kfree(memcg_lrus->lru[i]);
 }
@@ -422,6 +425,8 @@ static int memcg_init_list_lru(struct list_lru *lru, bool 
memcg_aware)
}
return 0;
 fail:
+   if (unlikely(i == 0))
+   return -ENOMEM;
for (i = i - 1; i >= 0; i--) {
if (!lru->node[i].memcg_lrus)
continue;
@@ -456,6 +461,8 @@ static int memcg_update_list_lru(struct list_lru *lru,
}
return 0;
 fail:
+   if (unlikely(i == 0))
+   return -ENOMEM;
for (i = i - 1; i >= 0; i--) {
if (!lru->node[i].memcg_lrus)
continue;
-- 
2.10.1

[PATCH] mm: Add conditions to avoid out-of-bounds

[nixiaoming]

In the function memcg_init_list_lru
if call goto fail when i == 0, will cause out-of-bounds at lru->node[i]

The same out-of-bounds access scenario exists in the functions
memcg_update_list_lru and __memcg_init_list_lru_node

Signed-off-by: nixiaoming 
---
 mm/list_lru.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/mm/list_lru.c b/mm/list_lru.c
index fcfb6c8..ec6bdd9 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -298,6 +298,9 @@ static void __memcg_destroy_list_lru_node(struct 
list_lru_memcg *memcg_lrus,
 {
int i;
 
+   if (unlikely(begin >= end))
+   return;
+
for (i = begin; i < end; i++)
kfree(memcg_lrus->lru[i]);
 }
@@ -422,6 +425,8 @@ static int memcg_init_list_lru(struct list_lru *lru, bool 
memcg_aware)
}
return 0;
 fail:
+   if (unlikely(i == 0))
+   return -ENOMEM;
for (i = i - 1; i >= 0; i--) {
if (!lru->node[i].memcg_lrus)
continue;
@@ -456,6 +461,8 @@ static int memcg_update_list_lru(struct list_lru *lru,
}
return 0;
 fail:
+   if (unlikely(i == 0))
+   return -ENOMEM;
for (i = i - 1; i >= 0; i--) {
if (!lru->node[i].memcg_lrus)
continue;
-- 
2.10.1

[PATCH] net/dns_resolver: dns_query Modify parameter checking to avoid dead code

[nixiaoming]

After commit 1a4240f4764a ("DNS: Separate out CIFS DNS Resolver code")
a dead code exists in function dns_query

code show as below:
if (!name || namelen == 0)
return -EINVAL;
/*Now the value of "namelen" cannot be equal to 0*/

if (!namelen) /*The condition "!namelen"" cannot be true*/
namelen = strnlen(name, 256); /*deadcode*/

Modify parameter checking to avoid dead code

Signed-off-by: nixiaoming 
---
 net/dns_resolver/dns_query.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/dns_resolver/dns_query.c b/net/dns_resolver/dns_query.c
index 49da670..f2acee2 100644
--- a/net/dns_resolver/dns_query.c
+++ b/net/dns_resolver/dns_query.c
@@ -81,7 +81,9 @@ int dns_query(const char *type, const char *name, size_t 
namelen,
kenter("%s,%*.*s,%zu,%s",
   type, (int)namelen, (int)namelen, name, namelen, options);
 
-   if (!name || namelen == 0)
+   if (!name || namelen < 3 || namelen > 255)
+   return -EINVAL;
+   if (namelen > strnlen(name, 256)) /*maybe only need part of name*/
return -EINVAL;
 
/* construct the query key description as "[:]" */
@@ -94,10 +96,6 @@ int dns_query(const char *type, const char *name, size_t 
namelen,
desclen += typelen + 1;
}
 
-   if (!namelen)
-   namelen = strnlen(name, 256);
-   if (namelen < 3 || namelen > 255)
-   return -EINVAL;
desclen += namelen + 1;
 
desc = kmalloc(desclen, GFP_KERNEL);
-- 
2.10.1

[PATCH] net/dns_resolver: dns_query Modify parameter checking to avoid dead code

[nixiaoming]

After commit 1a4240f4764a ("DNS: Separate out CIFS DNS Resolver code")
a dead code exists in function dns_query

code show as below:
if (!name || namelen == 0)
return -EINVAL;
/*Now the value of "namelen" cannot be equal to 0*/

if (!namelen) /*The condition "!namelen"" cannot be true*/
namelen = strnlen(name, 256); /*deadcode*/

Modify parameter checking to avoid dead code

Signed-off-by: nixiaoming 
---
 net/dns_resolver/dns_query.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/dns_resolver/dns_query.c b/net/dns_resolver/dns_query.c
index 49da670..f2acee2 100644
--- a/net/dns_resolver/dns_query.c
+++ b/net/dns_resolver/dns_query.c
@@ -81,7 +81,9 @@ int dns_query(const char *type, const char *name, size_t 
namelen,
kenter("%s,%*.*s,%zu,%s",
   type, (int)namelen, (int)namelen, name, namelen, options);
 
-   if (!name || namelen == 0)
+   if (!name || namelen < 3 || namelen > 255)
+   return -EINVAL;
+   if (namelen > strnlen(name, 256)) /*maybe only need part of name*/
return -EINVAL;
 
/* construct the query key description as "[:]" */
@@ -94,10 +96,6 @@ int dns_query(const char *type, const char *name, size_t 
namelen,
desclen += typelen + 1;
}
 
-   if (!namelen)
-   namelen = strnlen(name, 256);
-   if (namelen < 3 || namelen > 255)
-   return -EINVAL;
desclen += namelen + 1;
 
desc = kmalloc(desclen, GFP_KERNEL);
-- 
2.10.1

For help, whether to check the return value of sata_scr_read should be added in sata_print_link_status

[Nixiaoming]

Hello
I have trouble reading the code, I hope you can help guide

The function sata_print_link_status in the file drivers/ata/libata-core.c 
checks the return value when the function sata_scr_read is called on line 3009, 
but does not check the return value when calling sata_scr_read on line 3011.
Why are two calls handled differently?
Is there any other code logic that guarantees that the 3011 line will not 
return an exception? 

drivers/ata/libata-core.c :
3005 static void sata_print_link_status(struct ata_link *link)
3006 {
3007 u32 sstatus, scontrol, tmp;
3008
3009 if (sata_scr_read(link, SCR_STATUS, ))
3010 return;
3011 sata_scr_read(link, SCR_CONTROL, );
3012
3013 if (ata_phys_link_online(link)) {

Thanks

For help, whether to check the return value of sata_scr_read should be added in sata_print_link_status

[Nixiaoming]

Hello
I have trouble reading the code, I hope you can help guide

The function sata_print_link_status in the file drivers/ata/libata-core.c 
checks the return value when the function sata_scr_read is called on line 3009, 
but does not check the return value when calling sata_scr_read on line 3011.
Why are two calls handled differently?
Is there any other code logic that guarantees that the 3011 line will not 
return an exception? 

drivers/ata/libata-core.c :
3005 static void sata_print_link_status(struct ata_link *link)
3006 {
3007 u32 sstatus, scontrol, tmp;
3008
3009 if (sata_scr_read(link, SCR_STATUS, ))
3010 return;
3011 sata_scr_read(link, SCR_CONTROL, );
3012
3013 if (ata_phys_link_online(link)) {

Thanks

RE: [PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[Nixiaoming]

Because CONFIG_STRICT_KERNEL_RWX=n cannot be set by make menuconfig on 
arm64/x86/s390 architecture 
So, these three patches should not be necessary
Sorry to disturb everyone
Thank you for your guidance

Thanks

-Original Message-
From: Mark Rutland [mailto:mark.rutl...@arm.com] 
Sent: Wednesday, May 30, 2018 4:08 PM
To: Nixiaoming 
Cc: Will Deacon ; catalin.mari...@arm.com; 
ard.biesheu...@linaro.org; marc.zyng...@arm.com; james.mo...@arm.com; 
kristina.martse...@arm.com; steve.cap...@arm.com; t...@linutronix.de; 
mi...@redhat.com; h...@zytor.com; a...@linux-foundation.org; vba...@suse.cz; 
mho...@suse.com; dave.han...@linux.intel.com; dan.j.willi...@intel.com; 
kirill.shute...@linux.intel.com; zhang@linux.alibaba.com; 
schwidef...@de.ibm.com; heiko.carst...@de.ibm.com; gre...@linuxfoundation.org; 
linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; 
x...@kernel.org; linux-s...@vger.kernel.org
Subject: Re: [PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for 
mark_rodata_ro

On Wed, May 30, 2018 at 03:31:38AM +, Nixiaoming wrote:
> Unable to set CONFIG_STRICT_KERNEL_RWX=n by make menuconfig ARCH=arm64

Indeed. Making this mandatory was a deliberate decision, in part because this
allows simplification of code (e.g. removal of #ifdef guards).

> When reading the code, I feel it is more appropriate to add macro control
> here.

I must disagree. I do not think it makes sense to add an #ifdef for a
configuration option that is mandatory.

There are other places in the kernel that should behave differently if
CONFIG_STRICT_KERNEL_RWX were disabled, so this wouldn't be sufficient even if
we were to make CONFIG_STRICT_KERNEL_RWX optional. i.e. the #ifdef would give
the misleading impression that STRICT_KERNEL_RWX *could* be made optional, even
though this might not function correctly.

Having an #ifdef here makes the code more complicated and confusing, for the
benefit of a case which cannot occur.

Thanks,
Mark.

> -Original Message-
> From: Will Deacon [mailto:will.dea...@arm.com] 
> Sent: Tuesday, May 29, 2018 11:45 PM
> To: Nixiaoming 
> Cc: catalin.mari...@arm.com; ard.biesheu...@linaro.org; marc.zyng...@arm.com; 
> james.mo...@arm.com; kristina.martse...@arm.com; steve.cap...@arm.com; 
> t...@linutronix.de; mi...@redhat.com; h...@zytor.com; 
> a...@linux-foundation.org; vba...@suse.cz; mho...@suse.com; 
> dave.han...@linux.intel.com; dan.j.willi...@intel.com; 
> kirill.shute...@linux.intel.com; zhang@linux.alibaba.com; 
> schwidef...@de.ibm.com; heiko.carst...@de.ibm.com; 
> gre...@linuxfoundation.org; linux-kernel@vger.kernel.org; 
> linux-arm-ker...@lists.infradead.org; x...@kernel.org; 
> linux-s...@vger.kernel.org
> Subject: Re: [PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for 
> mark_rodata_ro
> 
> On Tue, May 29, 2018 at 09:36:15PM +0800, nixiaoming wrote:
> > mark_rodata_ro is only called by the function mark_readonly when
> > CONFIG_STRICT_KERNEL_RWX=y,
> > if CONFIG_STRICT_KERNEL_RWX is not set
> > a compile warning may be triggered: unused function
> 
> How are you achieving this configuration? In our Kconfig we select this
> unconditionally.
> 
> Will

RE: [PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[Nixiaoming]

Because CONFIG_STRICT_KERNEL_RWX=n cannot be set by make menuconfig on 
arm64/x86/s390 architecture 
So, these three patches should not be necessary
Sorry to disturb everyone
Thank you for your guidance

Thanks

-Original Message-
From: Mark Rutland [mailto:mark.rutl...@arm.com] 
Sent: Wednesday, May 30, 2018 4:08 PM
To: Nixiaoming 
Cc: Will Deacon ; catalin.mari...@arm.com; 
ard.biesheu...@linaro.org; marc.zyng...@arm.com; james.mo...@arm.com; 
kristina.martse...@arm.com; steve.cap...@arm.com; t...@linutronix.de; 
mi...@redhat.com; h...@zytor.com; a...@linux-foundation.org; vba...@suse.cz; 
mho...@suse.com; dave.han...@linux.intel.com; dan.j.willi...@intel.com; 
kirill.shute...@linux.intel.com; zhang@linux.alibaba.com; 
schwidef...@de.ibm.com; heiko.carst...@de.ibm.com; gre...@linuxfoundation.org; 
linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; 
x...@kernel.org; linux-s...@vger.kernel.org
Subject: Re: [PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for 
mark_rodata_ro

On Wed, May 30, 2018 at 03:31:38AM +, Nixiaoming wrote:
> Unable to set CONFIG_STRICT_KERNEL_RWX=n by make menuconfig ARCH=arm64

Indeed. Making this mandatory was a deliberate decision, in part because this
allows simplification of code (e.g. removal of #ifdef guards).

> When reading the code, I feel it is more appropriate to add macro control
> here.

I must disagree. I do not think it makes sense to add an #ifdef for a
configuration option that is mandatory.

There are other places in the kernel that should behave differently if
CONFIG_STRICT_KERNEL_RWX were disabled, so this wouldn't be sufficient even if
we were to make CONFIG_STRICT_KERNEL_RWX optional. i.e. the #ifdef would give
the misleading impression that STRICT_KERNEL_RWX *could* be made optional, even
though this might not function correctly.

Having an #ifdef here makes the code more complicated and confusing, for the
benefit of a case which cannot occur.

Thanks,
Mark.

> -Original Message-
> From: Will Deacon [mailto:will.dea...@arm.com] 
> Sent: Tuesday, May 29, 2018 11:45 PM
> To: Nixiaoming 
> Cc: catalin.mari...@arm.com; ard.biesheu...@linaro.org; marc.zyng...@arm.com; 
> james.mo...@arm.com; kristina.martse...@arm.com; steve.cap...@arm.com; 
> t...@linutronix.de; mi...@redhat.com; h...@zytor.com; 
> a...@linux-foundation.org; vba...@suse.cz; mho...@suse.com; 
> dave.han...@linux.intel.com; dan.j.willi...@intel.com; 
> kirill.shute...@linux.intel.com; zhang@linux.alibaba.com; 
> schwidef...@de.ibm.com; heiko.carst...@de.ibm.com; 
> gre...@linuxfoundation.org; linux-kernel@vger.kernel.org; 
> linux-arm-ker...@lists.infradead.org; x...@kernel.org; 
> linux-s...@vger.kernel.org
> Subject: Re: [PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for 
> mark_rodata_ro
> 
> On Tue, May 29, 2018 at 09:36:15PM +0800, nixiaoming wrote:
> > mark_rodata_ro is only called by the function mark_readonly when
> > CONFIG_STRICT_KERNEL_RWX=y,
> > if CONFIG_STRICT_KERNEL_RWX is not set
> > a compile warning may be triggered: unused function
> 
> How are you achieving this configuration? In our Kconfig we select this
> unconditionally.
> 
> Will

RE: [PATCH 2/3] x86:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[Nixiaoming]

On 30 May 2018 at 2:07PM Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
wrote:

>On 30 May 2018 at 07:58, Ingo Molnar  wrote:
>>
>> * nixiaoming  wrote:
>>
>>> mark_rodata_ro is only called by the function mark_readonly
>>> when CONFIG_STRICT_KERNEL_RWX=y
>>>
>>> if CONFIG_STRICT_KERNEL_RWX is not set
>>> a compile warning may be triggered: unused function
>
>>
>> NAK, this is very ugly and the changelog doesn't appear to be true: the build
>> warning does not trigger in the default build, correct?
>>
>
>I don't see how the build warning could trigger at all, given that
>mark_rodata_ro() has external linkage.
>

Unable to set CONFIG_STRICT_KERNEL_RWX=n by make menuconfig ARCH=x86_64
the build warning does not trigger in the default build, 
but it should be more appropriate to add CONFIG control to the code.

RE: [PATCH 2/3] x86:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[Nixiaoming]

On 30 May 2018 at 2:07PM Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
wrote:

>On 30 May 2018 at 07:58, Ingo Molnar  wrote:
>>
>> * nixiaoming  wrote:
>>
>>> mark_rodata_ro is only called by the function mark_readonly
>>> when CONFIG_STRICT_KERNEL_RWX=y
>>>
>>> if CONFIG_STRICT_KERNEL_RWX is not set
>>> a compile warning may be triggered: unused function
>
>>
>> NAK, this is very ugly and the changelog doesn't appear to be true: the build
>> warning does not trigger in the default build, correct?
>>
>
>I don't see how the build warning could trigger at all, given that
>mark_rodata_ro() has external linkage.
>

Unable to set CONFIG_STRICT_KERNEL_RWX=n by make menuconfig ARCH=x86_64
the build warning does not trigger in the default build, 
but it should be more appropriate to add CONFIG control to the code.

RE: [PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[Nixiaoming]

Unable to set CONFIG_STRICT_KERNEL_RWX=n by make menuconfig ARCH=arm64

When reading the code, I feel it is more appropriate to add macro control here.


-Original Message-
From: Will Deacon [mailto:will.dea...@arm.com] 
Sent: Tuesday, May 29, 2018 11:45 PM
To: Nixiaoming 
Cc: catalin.mari...@arm.com; ard.biesheu...@linaro.org; marc.zyng...@arm.com; 
james.mo...@arm.com; kristina.martse...@arm.com; steve.cap...@arm.com; 
t...@linutronix.de; mi...@redhat.com; h...@zytor.com; 
a...@linux-foundation.org; vba...@suse.cz; mho...@suse.com; 
dave.han...@linux.intel.com; dan.j.willi...@intel.com; 
kirill.shute...@linux.intel.com; zhang@linux.alibaba.com; 
schwidef...@de.ibm.com; heiko.carst...@de.ibm.com; gre...@linuxfoundation.org; 
linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; 
x...@kernel.org; linux-s...@vger.kernel.org
Subject: Re: [PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for 
mark_rodata_ro

On Tue, May 29, 2018 at 09:36:15PM +0800, nixiaoming wrote:
> mark_rodata_ro is only called by the function mark_readonly when
> CONFIG_STRICT_KERNEL_RWX=y,
> if CONFIG_STRICT_KERNEL_RWX is not set
> a compile warning may be triggered: unused function

How are you achieving this configuration? In our Kconfig we select this
unconditionally.

Will

RE: [PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[Nixiaoming]

Unable to set CONFIG_STRICT_KERNEL_RWX=n by make menuconfig ARCH=arm64

When reading the code, I feel it is more appropriate to add macro control here.


-Original Message-
From: Will Deacon [mailto:will.dea...@arm.com] 
Sent: Tuesday, May 29, 2018 11:45 PM
To: Nixiaoming 
Cc: catalin.mari...@arm.com; ard.biesheu...@linaro.org; marc.zyng...@arm.com; 
james.mo...@arm.com; kristina.martse...@arm.com; steve.cap...@arm.com; 
t...@linutronix.de; mi...@redhat.com; h...@zytor.com; 
a...@linux-foundation.org; vba...@suse.cz; mho...@suse.com; 
dave.han...@linux.intel.com; dan.j.willi...@intel.com; 
kirill.shute...@linux.intel.com; zhang@linux.alibaba.com; 
schwidef...@de.ibm.com; heiko.carst...@de.ibm.com; gre...@linuxfoundation.org; 
linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org; 
x...@kernel.org; linux-s...@vger.kernel.org
Subject: Re: [PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for 
mark_rodata_ro

On Tue, May 29, 2018 at 09:36:15PM +0800, nixiaoming wrote:
> mark_rodata_ro is only called by the function mark_readonly when
> CONFIG_STRICT_KERNEL_RWX=y,
> if CONFIG_STRICT_KERNEL_RWX is not set
> a compile warning may be triggered: unused function

How are you achieving this configuration? In our Kconfig we select this
unconditionally.

Will

[PATCH 2/3] x86:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[nixiaoming]

mark_rodata_ro is only called by the function mark_readonly
when CONFIG_STRICT_KERNEL_RWX=y

if CONFIG_STRICT_KERNEL_RWX is not set
a compile warning may be triggered: unused function

Signed-off-by: nixiaoming 
---
 arch/x86/mm/init_32.c | 2 ++
 arch/x86/mm/init_64.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index c893c6a..121c567 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -920,6 +920,7 @@ static void mark_nxdata_nx(void)
set_pages_nx(virt_to_page(start), size >> PAGE_SHIFT);
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long start = PFN_ALIGN(_text);
@@ -957,3 +958,4 @@ void mark_rodata_ro(void)
if (__supported_pte_mask & _PAGE_NX)
debug_checkwx();
 }
+#endif /*end of CONFIG_STRICT_KERNEL_RWX*/
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 0a40060..1b7a1a7 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -1245,6 +1245,7 @@ void set_kernel_text_ro(void)
set_memory_ro(start, (end - start) >> PAGE_SHIFT);
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long start = PFN_ALIGN(_text);
@@ -1298,6 +1299,7 @@ void mark_rodata_ro(void)
 */
pti_clone_kernel_text();
 }
+#endif
 
 int kern_addr_valid(unsigned long addr)
 {
-- 
2.10.1

[PATCH 2/3] x86:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[nixiaoming]

mark_rodata_ro is only called by the function mark_readonly
when CONFIG_STRICT_KERNEL_RWX=y

if CONFIG_STRICT_KERNEL_RWX is not set
a compile warning may be triggered: unused function

Signed-off-by: nixiaoming 
---
 arch/x86/mm/init_32.c | 2 ++
 arch/x86/mm/init_64.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index c893c6a..121c567 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -920,6 +920,7 @@ static void mark_nxdata_nx(void)
set_pages_nx(virt_to_page(start), size >> PAGE_SHIFT);
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long start = PFN_ALIGN(_text);
@@ -957,3 +958,4 @@ void mark_rodata_ro(void)
if (__supported_pte_mask & _PAGE_NX)
debug_checkwx();
 }
+#endif /*end of CONFIG_STRICT_KERNEL_RWX*/
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 0a40060..1b7a1a7 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -1245,6 +1245,7 @@ void set_kernel_text_ro(void)
set_memory_ro(start, (end - start) >> PAGE_SHIFT);
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long start = PFN_ALIGN(_text);
@@ -1298,6 +1299,7 @@ void mark_rodata_ro(void)
 */
pti_clone_kernel_text();
 }
+#endif
 
 int kern_addr_valid(unsigned long addr)
 {
-- 
2.10.1

[PATCH 3/3] s390:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[nixiaoming]

mark_rodata_ro is only called by the function mark_readonly
when CONFIG_STRICT_KERNEL_RWX=y

if CONFIG_STRICT_KERNEL_RWX is not set
a compile warning may be triggered: unused function

Signed-off-by: nixiaoming 
---
 arch/s390/mm/init.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 3fa3e53..a96fc3f 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -116,6 +116,7 @@ void __init paging_init(void)
free_area_init_nodes(max_zone_pfns);
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long size = __end_ro_after_init - __start_ro_after_init;
@@ -123,6 +124,7 @@ void mark_rodata_ro(void)
set_memory_ro((unsigned long)__start_ro_after_init, size >> PAGE_SHIFT);
pr_info("Write protected read-only-after-init data: %luk\n", size >> 
10);
 }
+#endif
 
 void __init mem_init(void)
 {
-- 
2.10.1

[PATCH 3/3] s390:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[nixiaoming]

mark_rodata_ro is only called by the function mark_readonly
when CONFIG_STRICT_KERNEL_RWX=y

if CONFIG_STRICT_KERNEL_RWX is not set
a compile warning may be triggered: unused function

Signed-off-by: nixiaoming 
---
 arch/s390/mm/init.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 3fa3e53..a96fc3f 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -116,6 +116,7 @@ void __init paging_init(void)
free_area_init_nodes(max_zone_pfns);
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long size = __end_ro_after_init - __start_ro_after_init;
@@ -123,6 +124,7 @@ void mark_rodata_ro(void)
set_memory_ro((unsigned long)__start_ro_after_init, size >> PAGE_SHIFT);
pr_info("Write protected read-only-after-init data: %luk\n", size >> 
10);
 }
+#endif
 
 void __init mem_init(void)
 {
-- 
2.10.1

[PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[nixiaoming]

mark_rodata_ro is only called by the function mark_readonly when
CONFIG_STRICT_KERNEL_RWX=y,
if CONFIG_STRICT_KERNEL_RWX is not set
a compile warning may be triggered: unused function

Signed-off-by: nixiaoming 
---
 arch/arm64/mm/mmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 2dbb2c9..849f326 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -491,6 +491,7 @@ static void __init map_mem(pgd_t *pgdp)
 #endif
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long section_size;
@@ -505,6 +506,7 @@ void mark_rodata_ro(void)
 
debug_checkwx();
 }
+#endif
 
 static void __init map_kernel_segment(pgd_t *pgdp, void *va_start, void 
*va_end,
  pgprot_t prot, struct vm_struct *vma,
-- 
2.10.1

[PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[nixiaoming]

mark_rodata_ro is only called by the function mark_readonly when
CONFIG_STRICT_KERNEL_RWX=y,
if CONFIG_STRICT_KERNEL_RWX is not set
a compile warning may be triggered: unused function

Signed-off-by: nixiaoming 
---
 arch/arm64/mm/mmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 2dbb2c9..849f326 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -491,6 +491,7 @@ static void __init map_mem(pgd_t *pgdp)
 #endif
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long section_size;
@@ -505,6 +506,7 @@ void mark_rodata_ro(void)
 
debug_checkwx();
 }
+#endif
 
 static void __init map_kernel_segment(pgd_t *pgdp, void *va_start, void 
*va_end,
  pgprot_t prot, struct vm_struct *vma,
-- 
2.10.1

[PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[nixiaoming]

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
---
 arch/arm64/mm/mmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 2dbb2c9..849f326 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -491,6 +491,7 @@ static void __init map_mem(pgd_t *pgdp)
 #endif
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long section_size;
@@ -505,6 +506,7 @@ void mark_rodata_ro(void)
 
debug_checkwx();
 }
+#endif
 
 static void __init map_kernel_segment(pgd_t *pgdp, void *va_start, void 
*va_end,
  pgprot_t prot, struct vm_struct *vma,
-- 
2.10.1

[PATCH 2/3] x86:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[nixiaoming]

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
---
 arch/x86/mm/init_32.c | 2 ++
 arch/x86/mm/init_64.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index c893c6a..121c567 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -920,6 +920,7 @@ static void mark_nxdata_nx(void)
set_pages_nx(virt_to_page(start), size >> PAGE_SHIFT);
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long start = PFN_ALIGN(_text);
@@ -957,3 +958,4 @@ void mark_rodata_ro(void)
if (__supported_pte_mask & _PAGE_NX)
debug_checkwx();
 }
+#endif /*end of CONFIG_STRICT_KERNEL_RWX*/
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 0a40060..1b7a1a7 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -1245,6 +1245,7 @@ void set_kernel_text_ro(void)
set_memory_ro(start, (end - start) >> PAGE_SHIFT);
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long start = PFN_ALIGN(_text);
@@ -1298,6 +1299,7 @@ void mark_rodata_ro(void)
 */
pti_clone_kernel_text();
 }
+#endif
 
 int kern_addr_valid(unsigned long addr)
 {
-- 
2.10.1

[PATCH 1/3] arm64:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[nixiaoming]

Signed-off-by: nixiaoming 
---
 arch/arm64/mm/mmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 2dbb2c9..849f326 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -491,6 +491,7 @@ static void __init map_mem(pgd_t *pgdp)
 #endif
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long section_size;
@@ -505,6 +506,7 @@ void mark_rodata_ro(void)
 
debug_checkwx();
 }
+#endif
 
 static void __init map_kernel_segment(pgd_t *pgdp, void *va_start, void 
*va_end,
  pgprot_t prot, struct vm_struct *vma,
-- 
2.10.1

[PATCH 2/3] x86:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[nixiaoming]

Signed-off-by: nixiaoming 
---
 arch/x86/mm/init_32.c | 2 ++
 arch/x86/mm/init_64.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index c893c6a..121c567 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -920,6 +920,7 @@ static void mark_nxdata_nx(void)
set_pages_nx(virt_to_page(start), size >> PAGE_SHIFT);
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long start = PFN_ALIGN(_text);
@@ -957,3 +958,4 @@ void mark_rodata_ro(void)
if (__supported_pte_mask & _PAGE_NX)
debug_checkwx();
 }
+#endif /*end of CONFIG_STRICT_KERNEL_RWX*/
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 0a40060..1b7a1a7 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -1245,6 +1245,7 @@ void set_kernel_text_ro(void)
set_memory_ro(start, (end - start) >> PAGE_SHIFT);
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long start = PFN_ALIGN(_text);
@@ -1298,6 +1299,7 @@ void mark_rodata_ro(void)
 */
pti_clone_kernel_text();
 }
+#endif
 
 int kern_addr_valid(unsigned long addr)
 {
-- 
2.10.1

[PATCH 3/3] s390:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[nixiaoming]

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
---
 arch/s390/mm/init.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 3fa3e53..a96fc3f 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -116,6 +116,7 @@ void __init paging_init(void)
free_area_init_nodes(max_zone_pfns);
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long size = __end_ro_after_init - __start_ro_after_init;
@@ -123,6 +124,7 @@ void mark_rodata_ro(void)
set_memory_ro((unsigned long)__start_ro_after_init, size >> PAGE_SHIFT);
pr_info("Write protected read-only-after-init data: %luk\n", size >> 
10);
 }
+#endif
 
 void __init mem_init(void)
 {
-- 
2.10.1

[PATCH 3/3] s390:add missing CONFIG_STRICT_KERNEL_RWX for mark_rodata_ro

[nixiaoming]

Signed-off-by: nixiaoming 
---
 arch/s390/mm/init.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 3fa3e53..a96fc3f 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -116,6 +116,7 @@ void __init paging_init(void)
free_area_init_nodes(max_zone_pfns);
 }
 
+#ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void)
 {
unsigned long size = __end_ro_after_init - __start_ro_after_init;
@@ -123,6 +124,7 @@ void mark_rodata_ro(void)
set_memory_ro((unsigned long)__start_ro_after_init, size >> PAGE_SHIFT);
pr_info("Write protected read-only-after-init data: %luk\n", size >> 
10);
 }
+#endif
 
 void __init mem_init(void)
 {
-- 
2.10.1

[PATCH] scripts: Fixed printf format mismatch

[nixiaoming]

scripts/kallsyms.c: function write_src:
"printf", the #1 format specifier "d" need arg type "int",
but the according arg "table_cnt" has type "unsigned int"

scripts/recordmcount.c: function do_file:
"fprintf", the #1 format specifier "d" need arg type "int",
but the according arg "(*w2)(ehdr->e_machine)" has type "unsigned int"

scripts/recordmcount.h: function find_secsym_ndx:
"fprintf", the #1 format specifier "d" need arg type "int",
but the according arg "txtndx" has type "unsigned int"

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
---
 scripts/kallsyms.c | 2 +-
 scripts/recordmcount.c | 2 +-
 scripts/recordmcount.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 5abfbf1..e0c416a 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -424,7 +424,7 @@ static void write_src(void)
}
 
output_label("kallsyms_num_syms");
-   printf("\tPTR\t%d\n", table_cnt);
+   printf("\tPTR\t%u\n", table_cnt);
printf("\n");
 
/* table of offset markers, that give the offset in the compressed 
stream
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index 8c9691c..895c40e 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -500,7 +500,7 @@ do_file(char const *const fname)
gpfx = 0;
switch (w2(ehdr->e_machine)) {
default:
-   fprintf(stderr, "unrecognized e_machine %d %s\n",
+   fprintf(stderr, "unrecognized e_machine %u %s\n",
w2(ehdr->e_machine), fname);
fail_file();
break;
diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
index b9897e2..2e77937 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -441,7 +441,7 @@ static unsigned find_secsym_ndx(unsigned const txtndx,
return symp - sym0;
}
}
-   fprintf(stderr, "Cannot find symbol for section %d: %s.\n",
+   fprintf(stderr, "Cannot find symbol for section %u: %s.\n",
txtndx, txtname);
fail_file();
 }
-- 
2.10.1

[PATCH] scripts: Fixed printf format mismatch

[nixiaoming]

scripts/kallsyms.c: function write_src:
"printf", the #1 format specifier "d" need arg type "int",
but the according arg "table_cnt" has type "unsigned int"

scripts/recordmcount.c: function do_file:
"fprintf", the #1 format specifier "d" need arg type "int",
but the according arg "(*w2)(ehdr->e_machine)" has type "unsigned int"

scripts/recordmcount.h: function find_secsym_ndx:
"fprintf", the #1 format specifier "d" need arg type "int",
but the according arg "txtndx" has type "unsigned int"

Signed-off-by: nixiaoming 
---
 scripts/kallsyms.c | 2 +-
 scripts/recordmcount.c | 2 +-
 scripts/recordmcount.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 5abfbf1..e0c416a 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -424,7 +424,7 @@ static void write_src(void)
}
 
output_label("kallsyms_num_syms");
-   printf("\tPTR\t%d\n", table_cnt);
+   printf("\tPTR\t%u\n", table_cnt);
printf("\n");
 
/* table of offset markers, that give the offset in the compressed 
stream
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index 8c9691c..895c40e 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -500,7 +500,7 @@ do_file(char const *const fname)
gpfx = 0;
switch (w2(ehdr->e_machine)) {
default:
-   fprintf(stderr, "unrecognized e_machine %d %s\n",
+   fprintf(stderr, "unrecognized e_machine %u %s\n",
w2(ehdr->e_machine), fname);
fail_file();
break;
diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
index b9897e2..2e77937 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -441,7 +441,7 @@ static unsigned find_secsym_ndx(unsigned const txtndx,
return symp - sym0;
}
}
-   fprintf(stderr, "Cannot find symbol for section %d: %s.\n",
+   fprintf(stderr, "Cannot find symbol for section %u: %s.\n",
txtndx, txtname);
fail_file();
 }
-- 
2.10.1

[PATCH] scripts/dtc: Fixed format mismatch in fprintf

[nixiaoming]

format specifier "d" need arg type "int" , but the according arg
"fdt32_to_cpu(xxx)" has type "unsigned int"

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
---
 scripts/dtc/fdtdump.c  | 6 +++---
 scripts/dtc/flattree.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/dtc/fdtdump.c b/scripts/dtc/fdtdump.c
index 7d460a5..851bb6f 100644
--- a/scripts/dtc/fdtdump.c
+++ b/scripts/dtc/fdtdump.c
@@ -66,12 +66,12 @@ static void dump_blob(void *blob)
 
printf("/dts-v1/;\n");
printf("// magic:\t\t0x%x\n", fdt32_to_cpu(bph->magic));
-   printf("// totalsize:\t\t0x%x (%d)\n", totalsize, totalsize);
+   printf("// totalsize:\t\t0x%x (%u)\n", totalsize, totalsize);
printf("// off_dt_struct:\t0x%x\n", off_dt);
printf("// off_dt_strings:\t0x%x\n", off_str);
printf("// off_mem_rsvmap:\t0x%x\n", off_mem_rsvmap);
-   printf("// version:\t\t%d\n", version);
-   printf("// last_comp_version:\t%d\n",
+   printf("// version:\t\t%u\n", version);
+   printf("// last_comp_version:\t%u\n",
   fdt32_to_cpu(bph->last_comp_version));
if (version >= 2)
printf("// boot_cpuid_phys:\t0x%x\n",
diff --git a/scripts/dtc/flattree.c b/scripts/dtc/flattree.c
index 8d268fb..28da281 100644
--- a/scripts/dtc/flattree.c
+++ b/scripts/dtc/flattree.c
@@ -393,7 +393,7 @@ void dt_to_blob(FILE *f, struct dt_info *dti, int version)
padlen = 0;
if (quiet < 1)
fprintf(stderr,
-   "Warning: blob size %d >= minimum size 
%d\n",
+   "Warning: blob size %u >= minimum size 
%d\n",
fdt32_to_cpu(fdt.totalsize), minsize);
}
}
-- 
2.10.1

[PATCH] scripts/dtc: Fixed format mismatch in fprintf

[nixiaoming]

format specifier "d" need arg type "int" , but the according arg
"fdt32_to_cpu(xxx)" has type "unsigned int"

Signed-off-by: nixiaoming 
---
 scripts/dtc/fdtdump.c  | 6 +++---
 scripts/dtc/flattree.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/dtc/fdtdump.c b/scripts/dtc/fdtdump.c
index 7d460a5..851bb6f 100644
--- a/scripts/dtc/fdtdump.c
+++ b/scripts/dtc/fdtdump.c
@@ -66,12 +66,12 @@ static void dump_blob(void *blob)
 
printf("/dts-v1/;\n");
printf("// magic:\t\t0x%x\n", fdt32_to_cpu(bph->magic));
-   printf("// totalsize:\t\t0x%x (%d)\n", totalsize, totalsize);
+   printf("// totalsize:\t\t0x%x (%u)\n", totalsize, totalsize);
printf("// off_dt_struct:\t0x%x\n", off_dt);
printf("// off_dt_strings:\t0x%x\n", off_str);
printf("// off_mem_rsvmap:\t0x%x\n", off_mem_rsvmap);
-   printf("// version:\t\t%d\n", version);
-   printf("// last_comp_version:\t%d\n",
+   printf("// version:\t\t%u\n", version);
+   printf("// last_comp_version:\t%u\n",
   fdt32_to_cpu(bph->last_comp_version));
if (version >= 2)
printf("// boot_cpuid_phys:\t0x%x\n",
diff --git a/scripts/dtc/flattree.c b/scripts/dtc/flattree.c
index 8d268fb..28da281 100644
--- a/scripts/dtc/flattree.c
+++ b/scripts/dtc/flattree.c
@@ -393,7 +393,7 @@ void dt_to_blob(FILE *f, struct dt_info *dti, int version)
padlen = 0;
if (quiet < 1)
fprintf(stderr,
-   "Warning: blob size %d >= minimum size 
%d\n",
+   "Warning: blob size %u >= minimum size 
%d\n",
fdt32_to_cpu(fdt.totalsize), minsize);
}
}
-- 
2.10.1

[RESEND PATCH] fix memory leak in ramoops_init

[Nixiaoming]

1, memory leak in ramoops_register_dummy.
   dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
   but no free when platform_device_register_data return fail

2, if kzalloc(sizeof(*dummy_data), GFP_KERNEL) return NULL,
but platform_driver_register(_driver) return 0
   kfree(NULL) in ramoops_exit
so, add return val for ramoops_register_dummy, and check it in ramoops_init

3, memory leak in ramoops_init.
   miss platform_device_unregister(dummy) and kfree(dummy_data)
   when platform_driver_register(_driver) return fail

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
---
 fs/pstore/ram.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index bd9812e..331b600 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -604,17 +604,17 @@ static struct platform_driver ramoops_driver = {
},
 };
 
-static void ramoops_register_dummy(void)
+static int ramoops_register_dummy(void)
 {
if (!mem_size)
-   return;
+   return -EINVAL;
 
pr_info("using module parameters\n");
 
dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
if (!dummy_data) {
pr_info("could not allocate pdata\n");
-   return;
+   return -ENOMEM;
}
 
dummy_data->mem_size = mem_size;
@@ -636,13 +636,25 @@ static void ramoops_register_dummy(void)
if (IS_ERR(dummy)) {
pr_info("could not create platform device: %ld\n",
PTR_ERR(dummy));
+   kfree(dummy_data);
+   return PTR_ERR(dummy);
}
+   return 0;
 }
 
 static int __init ramoops_init(void)
 {
-   ramoops_register_dummy();
-   return platform_driver_register(_driver);
+   int ret = ramoops_register_dummy();
+
+   if (ret != 0)
+   return ret;
+
+   ret = platform_driver_register(_driver);
+   if (ret != 0) {
+   platform_device_unregister(dummy);
+   kfree(dummy_data);
+   }
+   return ret;
 }
 postcore_initcall(ramoops_init);
 
-- 
1.9.0

[RESEND PATCH] fix memory leak in ramoops_init

[Nixiaoming]

1, memory leak in ramoops_register_dummy.
   dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
   but no free when platform_device_register_data return fail

2, if kzalloc(sizeof(*dummy_data), GFP_KERNEL) return NULL,
but platform_driver_register(_driver) return 0
   kfree(NULL) in ramoops_exit
so, add return val for ramoops_register_dummy, and check it in ramoops_init

3, memory leak in ramoops_init.
   miss platform_device_unregister(dummy) and kfree(dummy_data)
   when platform_driver_register(_driver) return fail

Signed-off-by: nixiaoming 
---
 fs/pstore/ram.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index bd9812e..331b600 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -604,17 +604,17 @@ static struct platform_driver ramoops_driver = {
},
 };
 
-static void ramoops_register_dummy(void)
+static int ramoops_register_dummy(void)
 {
if (!mem_size)
-   return;
+   return -EINVAL;
 
pr_info("using module parameters\n");
 
dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
if (!dummy_data) {
pr_info("could not allocate pdata\n");
-   return;
+   return -ENOMEM;
}
 
dummy_data->mem_size = mem_size;
@@ -636,13 +636,25 @@ static void ramoops_register_dummy(void)
if (IS_ERR(dummy)) {
pr_info("could not create platform device: %ld\n",
PTR_ERR(dummy));
+   kfree(dummy_data);
+   return PTR_ERR(dummy);
}
+   return 0;
 }
 
 static int __init ramoops_init(void)
 {
-   ramoops_register_dummy();
-   return platform_driver_register(_driver);
+   int ret = ramoops_register_dummy();
+
+   if (ret != 0)
+   return ret;
+
+   ret = platform_driver_register(_driver);
+   if (ret != 0) {
+   platform_device_unregister(dummy);
+   kfree(dummy_data);
+   }
+   return ret;
 }
 postcore_initcall(ramoops_init);
 
-- 
1.9.0

[PATCH] fix memory leak in ramoops_init

[nixiaoming]

1, memory leak in ramoops_register_dummy.
   dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
   but no free when platform_device_register_data return fail

2, if kzalloc(sizeof(*dummy_data), GFP_KERNEL) return NULL,
but platform_driver_register(_driver) return 0
   kfree(NULL) in ramoops_exit
so, add return val for ramoops_register_dummy, and check it in ramoops_init

3, memory leak in ramoops_init.
   miss platform_device_unregister(dummy) and kfree(dummy_data)
   when platform_driver_register(_driver) return fail

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
---
 fs/pstore/ram.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index bd9812e..331b600 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -604,17 +604,17 @@ static struct platform_driver ramoops_driver = {
},
 };
 
-static void ramoops_register_dummy(void)
+static int ramoops_register_dummy(void)
 {
if (!mem_size)
-   return;
+   return -EINVAL;
 
pr_info("using module parameters\n");
 
dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
if (!dummy_data) {
pr_info("could not allocate pdata\n");
-   return;
+   return -ENOMEM;
}
 
dummy_data->mem_size = mem_size;
@@ -636,13 +636,25 @@ static void ramoops_register_dummy(void)
if (IS_ERR(dummy)) {
pr_info("could not create platform device: %ld\n",
PTR_ERR(dummy));
+   kfree(dummy_data);
+   return PTR_ERR(dummy);
}
+   return 0;
 }
 
 static int __init ramoops_init(void)
 {
-   ramoops_register_dummy();
-   return platform_driver_register(_driver);
+   int ret = ramoops_register_dummy();
+
+   if (ret != 0)
+   return ret;
+
+   ret = platform_driver_register(_driver);
+   if (ret != 0) {
+   platform_device_unregister(dummy);
+   kfree(dummy_data);
+   }
+   return ret;
 }
 postcore_initcall(ramoops_init);
 
-- 
1.9.0

[PATCH] fix memory leak in ramoops_init

[nixiaoming]

1, memory leak in ramoops_register_dummy.
   dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
   but no free when platform_device_register_data return fail

2, if kzalloc(sizeof(*dummy_data), GFP_KERNEL) return NULL,
but platform_driver_register(_driver) return 0
   kfree(NULL) in ramoops_exit
so, add return val for ramoops_register_dummy, and check it in ramoops_init

3, memory leak in ramoops_init.
   miss platform_device_unregister(dummy) and kfree(dummy_data)
   when platform_driver_register(_driver) return fail

Signed-off-by: nixiaoming 
---
 fs/pstore/ram.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index bd9812e..331b600 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -604,17 +604,17 @@ static struct platform_driver ramoops_driver = {
},
 };
 
-static void ramoops_register_dummy(void)
+static int ramoops_register_dummy(void)
 {
if (!mem_size)
-   return;
+   return -EINVAL;
 
pr_info("using module parameters\n");
 
dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
if (!dummy_data) {
pr_info("could not allocate pdata\n");
-   return;
+   return -ENOMEM;
}
 
dummy_data->mem_size = mem_size;
@@ -636,13 +636,25 @@ static void ramoops_register_dummy(void)
if (IS_ERR(dummy)) {
pr_info("could not create platform device: %ld\n",
PTR_ERR(dummy));
+   kfree(dummy_data);
+   return PTR_ERR(dummy);
}
+   return 0;
 }
 
 static int __init ramoops_init(void)
 {
-   ramoops_register_dummy();
-   return platform_driver_register(_driver);
+   int ret = ramoops_register_dummy();
+
+   if (ret != 0)
+   return ret;
+
+   ret = platform_driver_register(_driver);
+   if (ret != 0) {
+   platform_device_unregister(dummy);
+   kfree(dummy_data);
+   }
+   return ret;
 }
 postcore_initcall(ramoops_init);
 
-- 
1.9.0

RE: [PATCH] tty fix oops when rmmod 8250

[Nixiaoming]

Test on 4.14.0-rc4:

CPU: 7 PID: 449 Comm: rmmod Tainted: G   O4.14.0-rc4+ #1
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
Call Trace:
 dump_stack+0x50/0x80
 jtty_kref_put+0x5a/0x5c [jprobe_tty_kref_put]
 uart_remove_one_port+0xe8/0x220 [serial_core]
 ? __might_sleep+0x4a/0x90
 serial8250_unregister_port+0x71/0x100 [8250]
 serial_pnp_remove+0x26/0x30 [8250]
 pnp_device_remove+0x31/0x70
 device_release_driver_internal+0x185/0x240
 driver_detach+0x47/0x90
 bus_remove_driver+0x50/0xb0
 driver_unregister+0x30/0x50
 pnp_unregister_driver+0x12/0x20
 serial8250_pnp_exit+0x15/0x20 [8250]
 serial8250_exit+0x34/0xbf8 [8250]
 SyS_delete_module+0x17a/0x1f0
 ? exit_to_usermode_loop+0x9d/0xc0
 do_syscall_64+0x5c/0x120
 ? syscall_return_slowpath+0xb9/0xc0
 ? schedule_tail+0xc1/0xe0
 entry_SYSCALL64_slow_path+0x25/0x25
RIP: 0033:0x7ff7d37ab257
RSP: 002b:7ffdb7879f08 EFLAGS: 0202 ORIG_RAX: 00b0
RAX: ffda RBX: 0800 RCX: 7ff7d37ab257
RDX: 7ff7d38128c0 RSI: 0800 RDI: 006d60f0
RBP: 006d6090 R08: 7ff7d3a5bf40 R09: 7ffdb7878eb1
R10:  R11: 0202 R12: 7ffdb787be86
R13: 006d6010 R14:  R15: 006d6090
BUG: unable to handle kernel paging request at a00bdc91
IP: strchr+0x3/0x30
PGD 1c0b067 P4D 1c0b067 PUD 1c0c063 PMD 7f620067 PTE 0
Oops:  [#1] PREEMPT SMP
Modules linked in: jprobe_tty_kref_put(O) iptable_filter br_netfilter bridge 
stp llc ipv6 ata_piix ahci libahci libata ext4 jbd2 8250_base serial_core ptp 
pps_core nfsd auth_rpcgss oid_registry nfsv3 nfs nfs_acl lockd sunrpc grace 
vfat fat quota_v2 quota_v1 quota_tree [last unloaded: 8250]
CPU: 6 PID: 74 Comm: kworker/6:1 Tainted: G   O4.14.0-rc4+ #1
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
Workqueue: events release_one_tty
task: 8800dbb88000 task.stack: c98d
RIP: 0010:strchr+0x3/0x30
RSP: 0018:c98d3b38 EFLAGS: 00010286
RAX: 81c682c0 RBX: a00bdc91 RCX: 00018040003c
RDX: 002f RSI: 002f RDI: a00bdc91
RBP: c98d3b78 R08:  R09: 8140e1ed
R10: ea0001fd6b00 R11:  R12: 8800df412900
R13: 0010 R14: c98d3b90 R15: a00bdc91
FS:  () GS:8800dfb8() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: a00bdc91 CR3: 72b7a000 CR4: 06e0
Call Trace:
 ? __xlate_proc_name+0x66/0xb0
 remove_proc_entry+0x37/0x140
 proc_tty_unregister_driver+0x28/0x40
 destruct_tty_driver+0x84/0xe0
 tty_driver_kref_put+0x1e/0x30
 release_one_tty+0x62/0xe0
 process_one_work+0x1d0/0x440
 ? sched_clock_local+0x1c/0x90
 ? schedule+0x4e/0xc0
 ? preempt_count_add+0xaa/0xc0
 worker_thread+0x110/0x4c0
 ? __schedule+0x4ee/0x8b0
 ? default_wake_function+0x12/0x20
 ? __wake_up_common+0x85/0x130
 ? schedule+0x4e/0xc0
 kthread+0x13a/0x140
 ? process_one_work+0x440/0x440
 ? kthreadd+0x1c0/0x1c0
 ret_from_fork+0x22/0x30
Code: 01 41 38 c0 75 13 48 ff c1 45 84 c0 74 05 48 ff ca 75 e3 31 c0 c9 66 90 
c3 41 38 c0 c9 19 c0 83 c8 01 c3 0f 1f 44 00 00 55 89 f2 <0f> b6 07 48 89 e5 40 
38 f0 75 0c eb 12 48 ff c7 0f b6 07 38 d0 
RIP: strchr+0x3/0x30 RSP: c98d3b38
CR2: a00bdc91

-

RE: [PATCH] tty fix oops when rmmod 8250

[Nixiaoming]

Test on 4.14.0-rc4:

CPU: 7 PID: 449 Comm: rmmod Tainted: G   O4.14.0-rc4+ #1
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
Call Trace:
 dump_stack+0x50/0x80
 jtty_kref_put+0x5a/0x5c [jprobe_tty_kref_put]
 uart_remove_one_port+0xe8/0x220 [serial_core]
 ? __might_sleep+0x4a/0x90
 serial8250_unregister_port+0x71/0x100 [8250]
 serial_pnp_remove+0x26/0x30 [8250]
 pnp_device_remove+0x31/0x70
 device_release_driver_internal+0x185/0x240
 driver_detach+0x47/0x90
 bus_remove_driver+0x50/0xb0
 driver_unregister+0x30/0x50
 pnp_unregister_driver+0x12/0x20
 serial8250_pnp_exit+0x15/0x20 [8250]
 serial8250_exit+0x34/0xbf8 [8250]
 SyS_delete_module+0x17a/0x1f0
 ? exit_to_usermode_loop+0x9d/0xc0
 do_syscall_64+0x5c/0x120
 ? syscall_return_slowpath+0xb9/0xc0
 ? schedule_tail+0xc1/0xe0
 entry_SYSCALL64_slow_path+0x25/0x25
RIP: 0033:0x7ff7d37ab257
RSP: 002b:7ffdb7879f08 EFLAGS: 0202 ORIG_RAX: 00b0
RAX: ffda RBX: 0800 RCX: 7ff7d37ab257
RDX: 7ff7d38128c0 RSI: 0800 RDI: 006d60f0
RBP: 006d6090 R08: 7ff7d3a5bf40 R09: 7ffdb7878eb1
R10:  R11: 0202 R12: 7ffdb787be86
R13: 006d6010 R14:  R15: 006d6090
BUG: unable to handle kernel paging request at a00bdc91
IP: strchr+0x3/0x30
PGD 1c0b067 P4D 1c0b067 PUD 1c0c063 PMD 7f620067 PTE 0
Oops:  [#1] PREEMPT SMP
Modules linked in: jprobe_tty_kref_put(O) iptable_filter br_netfilter bridge 
stp llc ipv6 ata_piix ahci libahci libata ext4 jbd2 8250_base serial_core ptp 
pps_core nfsd auth_rpcgss oid_registry nfsv3 nfs nfs_acl lockd sunrpc grace 
vfat fat quota_v2 quota_v1 quota_tree [last unloaded: 8250]
CPU: 6 PID: 74 Comm: kworker/6:1 Tainted: G   O4.14.0-rc4+ #1
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
Workqueue: events release_one_tty
task: 8800dbb88000 task.stack: c98d
RIP: 0010:strchr+0x3/0x30
RSP: 0018:c98d3b38 EFLAGS: 00010286
RAX: 81c682c0 RBX: a00bdc91 RCX: 00018040003c
RDX: 002f RSI: 002f RDI: a00bdc91
RBP: c98d3b78 R08:  R09: 8140e1ed
R10: ea0001fd6b00 R11:  R12: 8800df412900
R13: 0010 R14: c98d3b90 R15: a00bdc91
FS:  () GS:8800dfb8() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: a00bdc91 CR3: 72b7a000 CR4: 06e0
Call Trace:
 ? __xlate_proc_name+0x66/0xb0
 remove_proc_entry+0x37/0x140
 proc_tty_unregister_driver+0x28/0x40
 destruct_tty_driver+0x84/0xe0
 tty_driver_kref_put+0x1e/0x30
 release_one_tty+0x62/0xe0
 process_one_work+0x1d0/0x440
 ? sched_clock_local+0x1c/0x90
 ? schedule+0x4e/0xc0
 ? preempt_count_add+0xaa/0xc0
 worker_thread+0x110/0x4c0
 ? __schedule+0x4ee/0x8b0
 ? default_wake_function+0x12/0x20
 ? __wake_up_common+0x85/0x130
 ? schedule+0x4e/0xc0
 kthread+0x13a/0x140
 ? process_one_work+0x440/0x440
 ? kthreadd+0x1c0/0x1c0
 ret_from_fork+0x22/0x30
Code: 01 41 38 c0 75 13 48 ff c1 45 84 c0 74 05 48 ff ca 75 e3 31 c0 c9 66 90 
c3 41 38 c0 c9 19 c0 83 c8 01 c3 0f 1f 44 00 00 55 89 f2 <0f> b6 07 48 89 e5 40 
38 f0 75 0c eb 12 48 ff c7 0f b6 07 38 d0 
RIP: strchr+0x3/0x30 RSP: c98d3b38
CR2: a00bdc91

-

Re:Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook

[Nixiaoming]

On Fri, Sep 15, 2017 at 10:46 AM, Willem de Bruijn
 wrote:
>
> In case of failure we also need to unlink and free match. I
> sent the following:
>
> http://patchwork.ozlabs.org/patch/813945/

+   spin_lock(>bind_lock);
+   if (po->running &&
+   match->type == type &&
   match->prot_hook.type == po->prot_hook.type &&
   match->prot_hook.dev == po->prot_hook.dev) {
err = -ENOSPC;
@@ -1761,6 +1760,13 @@  static int fanout_add(struct sock *sk, u16 id, u16 
type_flags)
  err = 0;
}
   }
+   spin_unlock(>bind_lock);
+
+   if (err && !refcount_read(>sk_ref)) {
+list_del(>list);
+kfree(match);
+   }


In the function fanout_add add spin_lock to protect po-> running and po-> 
fanout, 
then whether it should be in the function fanout_release also add spin_lock 
protection ?

static struct packet_fanout *fanout_release(struct sock *sk)

mutex_lock(_mutex);
f = po->fanout;
if (f) {
po->fanout = NULL;

Re:Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook

[Nixiaoming]

On Fri, Sep 15, 2017 at 10:46 AM, Willem de Bruijn
 wrote:
>
> In case of failure we also need to unlink and free match. I
> sent the following:
>
> http://patchwork.ozlabs.org/patch/813945/

+   spin_lock(>bind_lock);
+   if (po->running &&
+   match->type == type &&
   match->prot_hook.type == po->prot_hook.type &&
   match->prot_hook.dev == po->prot_hook.dev) {
err = -ENOSPC;
@@ -1761,6 +1760,13 @@  static int fanout_add(struct sock *sk, u16 id, u16 
type_flags)
  err = 0;
}
   }
+   spin_unlock(>bind_lock);
+
+   if (err && !refcount_read(>sk_ref)) {
+list_del(>list);
+kfree(match);
+   }


In the function fanout_add add spin_lock to protect po-> running and po-> 
fanout, 
then whether it should be in the function fanout_release also add spin_lock 
protection ?

static struct packet_fanout *fanout_release(struct sock *sk)

mutex_lock(_mutex);
f = po->fanout;
if (f) {
po->fanout = NULL;

[PATCH] tty fix oops when rmmod 8250

[nixiaoming]

After rmmod 8250.ko
tty_kref_put starts kwork (release_one_tty) to release proc interface
oops when accessing driver->driver_name in proc_tty_unregister_driver

Use jprobe, found driver->driver_name point to 8250.ko
static static struct uart_driver serial8250_reg
.driver_name= serial,

Use name in proc_dir_entry instead of driver->driver_name to fix oops

test on linux 4.1.12:

BUG: unable to handle kernel paging request at a01979de
IP: [] strchr+0x0/0x30
PGD 1a0d067 PUD 1a0e063 PMD 851c1f067 PTE 0
Oops:  [#1] PREEMPT SMP
Modules linked in: ... ...  [last unloaded: 8250]
CPU: 7 PID: 116 Comm: kworker/7:1 Tainted: G   O4.1.12 #1
Hardware name: Insyde RiverForest/Type2 - Board Product Name1, BIOS NE5KV904 
12/21/2015
Workqueue: events release_one_tty
task: 88085b684960 ti: 880852884000 task.ti: 880852884000
RIP: 0010:[]  [] strchr+0x0/0x30
RSP: 0018:880852887c90  EFLAGS: 00010282
RAX: 81a5eca0 RBX: a01979de RCX: 0004
RDX: 880852887d10 RSI: 002f RDI: a01979de
RBP: 880852887cd8 R08:  R09: 88085f5d94d0
R10: 0195 R11:  R12: a01979de
R13: 880852887d00 R14: a01979de R15: 88085f02e840
FS:  () GS:88085f5c() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: a01979de CR3: 01a0c000 CR4: 001406e0
Stack:
 812349b1 880852887cb8 880852887d10 88085f5cd6c2
 880852800a80 a01979de 880852800a84 0010
 88085bb28bd8 880852887d38 812354f0 880852887d08
Call Trace:
 [] ? __xlate_proc_name+0x71/0xd0
 [] remove_proc_entry+0x40/0x180
 [] ? _raw_spin_lock_irqsave+0x41/0x60
 [] ? destruct_tty_driver+0x60/0xe0
 [] proc_tty_unregister_driver+0x28/0x40
 [] destruct_tty_driver+0x88/0xe0
 [] tty_driver_kref_put+0x1d/0x20
 [] release_one_tty+0x5a/0xd0
 [] process_one_work+0x139/0x420
 [] worker_thread+0x121/0x450
 [] ? process_scheduled_works+0x40/0x40
 [] kthread+0xec/0x110
 [] ? tg_rt_schedulable+0x210/0x220
 [] ? kthread_freezable_should_stop+0x80/0x80
 [] ret_from_fork+0x42/0x70
 [] ? kthread_freezable_should_stop+0x80/0x80

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
---
 fs/proc/proc_tty.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/proc/proc_tty.c b/fs/proc/proc_tty.c
index 901bd06..20e2c18 100644
--- a/fs/proc/proc_tty.c
+++ b/fs/proc/proc_tty.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include "internal.h"
 
 /*
  * The /proc/tty directory inodes...
@@ -164,7 +165,7 @@ void proc_tty_unregister_driver(struct tty_driver *driver)
if (!ent)
return;

-   remove_proc_entry(driver->driver_name, proc_tty_driver);
+   remove_proc_entry(ent->name, proc_tty_driver);

driver->proc_entry = NULL;
 }
-- 
2.11.0.1

[PATCH] tty fix oops when rmmod 8250

[nixiaoming]

After rmmod 8250.ko
tty_kref_put starts kwork (release_one_tty) to release proc interface
oops when accessing driver->driver_name in proc_tty_unregister_driver

Use jprobe, found driver->driver_name point to 8250.ko
static static struct uart_driver serial8250_reg
.driver_name= serial,

Use name in proc_dir_entry instead of driver->driver_name to fix oops

test on linux 4.1.12:

BUG: unable to handle kernel paging request at a01979de
IP: [] strchr+0x0/0x30
PGD 1a0d067 PUD 1a0e063 PMD 851c1f067 PTE 0
Oops:  [#1] PREEMPT SMP
Modules linked in: ... ...  [last unloaded: 8250]
CPU: 7 PID: 116 Comm: kworker/7:1 Tainted: G   O4.1.12 #1
Hardware name: Insyde RiverForest/Type2 - Board Product Name1, BIOS NE5KV904 
12/21/2015
Workqueue: events release_one_tty
task: 88085b684960 ti: 880852884000 task.ti: 880852884000
RIP: 0010:[]  [] strchr+0x0/0x30
RSP: 0018:880852887c90  EFLAGS: 00010282
RAX: 81a5eca0 RBX: a01979de RCX: 0004
RDX: 880852887d10 RSI: 002f RDI: a01979de
RBP: 880852887cd8 R08:  R09: 88085f5d94d0
R10: 0195 R11:  R12: a01979de
R13: 880852887d00 R14: a01979de R15: 88085f02e840
FS:  () GS:88085f5c() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: a01979de CR3: 01a0c000 CR4: 001406e0
Stack:
 812349b1 880852887cb8 880852887d10 88085f5cd6c2
 880852800a80 a01979de 880852800a84 0010
 88085bb28bd8 880852887d38 812354f0 880852887d08
Call Trace:
 [] ? __xlate_proc_name+0x71/0xd0
 [] remove_proc_entry+0x40/0x180
 [] ? _raw_spin_lock_irqsave+0x41/0x60
 [] ? destruct_tty_driver+0x60/0xe0
 [] proc_tty_unregister_driver+0x28/0x40
 [] destruct_tty_driver+0x88/0xe0
 [] tty_driver_kref_put+0x1d/0x20
 [] release_one_tty+0x5a/0xd0
 [] process_one_work+0x139/0x420
 [] worker_thread+0x121/0x450
 [] ? process_scheduled_works+0x40/0x40
 [] kthread+0xec/0x110
 [] ? tg_rt_schedulable+0x210/0x220
 [] ? kthread_freezable_should_stop+0x80/0x80
 [] ret_from_fork+0x42/0x70
 [] ? kthread_freezable_should_stop+0x80/0x80

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
---
 fs/proc/proc_tty.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/proc/proc_tty.c b/fs/proc/proc_tty.c
index 901bd06..20e2c18 100644
--- a/fs/proc/proc_tty.c
+++ b/fs/proc/proc_tty.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include "internal.h"
 
 /*
  * The /proc/tty directory inodes...
@@ -164,7 +165,7 @@ void proc_tty_unregister_driver(struct tty_driver *driver)
if (!ent)
return;

-   remove_proc_entry(driver->driver_name, proc_tty_driver);
+   remove_proc_entry(ent->name, proc_tty_driver);

driver->proc_entry = NULL;
 }
-- 
2.11.0.1

[PATCH] tty fix oops when rmmod 8250

[nixiaoming]

After rmmod 8250.ko
tty_kref_put starts kwork (release_one_tty) to release proc interface
oops when accessing driver->driver_name in proc_tty_unregister_driver

Use jprobe, found driver->driver_name point to 8250.ko
static static struct uart_driver serial8250_reg
.driver_name= serial,

Use name in proc_dir_entry instead of driver->driver_name to fix oops

test on linux 4.1.12:

BUG: unable to handle kernel paging request at a01979de
IP: [] strchr+0x0/0x30
PGD 1a0d067 PUD 1a0e063 PMD 851c1f067 PTE 0
Oops:  [#1] PREEMPT SMP
Modules linked in: ... ...  [last unloaded: 8250]
CPU: 7 PID: 116 Comm: kworker/7:1 Tainted: G   O4.1.12 #1
Hardware name: Insyde RiverForest/Type2 - Board Product Name1, BIOS NE5KV904 
12/21/2015
Workqueue: events release_one_tty
task: 88085b684960 ti: 880852884000 task.ti: 880852884000
RIP: 0010:[]  [] strchr+0x0/0x30
RSP: 0018:880852887c90  EFLAGS: 00010282
RAX: 81a5eca0 RBX: a01979de RCX: 0004
RDX: 880852887d10 RSI: 002f RDI: a01979de
RBP: 880852887cd8 R08:  R09: 88085f5d94d0
R10: 0195 R11:  R12: a01979de
R13: 880852887d00 R14: a01979de R15: 88085f02e840
FS:  () GS:88085f5c() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: a01979de CR3: 01a0c000 CR4: 001406e0
Stack:
 812349b1 880852887cb8 880852887d10 88085f5cd6c2
 880852800a80 a01979de 880852800a84 0010
 88085bb28bd8 880852887d38 812354f0 880852887d08
Call Trace:
 [] ? __xlate_proc_name+0x71/0xd0
 [] remove_proc_entry+0x40/0x180
 [] ? _raw_spin_lock_irqsave+0x41/0x60
 [] ? destruct_tty_driver+0x60/0xe0
 [] proc_tty_unregister_driver+0x28/0x40
 [] destruct_tty_driver+0x88/0xe0
 [] tty_driver_kref_put+0x1d/0x20
 [] release_one_tty+0x5a/0xd0
 [] process_one_work+0x139/0x420
 [] worker_thread+0x121/0x450
 [] ? process_scheduled_works+0x40/0x40
 [] kthread+0xec/0x110
 [] ? tg_rt_schedulable+0x210/0x220
 [] ? kthread_freezable_should_stop+0x80/0x80
 [] ret_from_fork+0x42/0x70
 [] ? kthread_freezable_should_stop+0x80/0x80

Signed-off-by: nixiaoming 
---
 fs/proc/proc_tty.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/proc/proc_tty.c b/fs/proc/proc_tty.c
index 901bd06..20e2c18 100644
--- a/fs/proc/proc_tty.c
+++ b/fs/proc/proc_tty.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include "internal.h"
 
 /*
  * The /proc/tty directory inodes...
@@ -164,7 +165,7 @@ void proc_tty_unregister_driver(struct tty_driver *driver)
if (!ent)
return;

-   remove_proc_entry(driver->driver_name, proc_tty_driver);
+   remove_proc_entry(ent->name, proc_tty_driver);

driver->proc_entry = NULL;
 }
-- 
2.11.0.1

[PATCH] tty fix oops when rmmod 8250

[nixiaoming]

After rmmod 8250.ko
tty_kref_put starts kwork (release_one_tty) to release proc interface
oops when accessing driver->driver_name in proc_tty_unregister_driver

Use jprobe, found driver->driver_name point to 8250.ko
static static struct uart_driver serial8250_reg
.driver_name= serial,

Use name in proc_dir_entry instead of driver->driver_name to fix oops

test on linux 4.1.12:

BUG: unable to handle kernel paging request at a01979de
IP: [] strchr+0x0/0x30
PGD 1a0d067 PUD 1a0e063 PMD 851c1f067 PTE 0
Oops:  [#1] PREEMPT SMP
Modules linked in: ... ...  [last unloaded: 8250]
CPU: 7 PID: 116 Comm: kworker/7:1 Tainted: G   O4.1.12 #1
Hardware name: Insyde RiverForest/Type2 - Board Product Name1, BIOS NE5KV904 
12/21/2015
Workqueue: events release_one_tty
task: 88085b684960 ti: 880852884000 task.ti: 880852884000
RIP: 0010:[]  [] strchr+0x0/0x30
RSP: 0018:880852887c90  EFLAGS: 00010282
RAX: 81a5eca0 RBX: a01979de RCX: 0004
RDX: 880852887d10 RSI: 002f RDI: a01979de
RBP: 880852887cd8 R08:  R09: 88085f5d94d0
R10: 0195 R11:  R12: a01979de
R13: 880852887d00 R14: a01979de R15: 88085f02e840
FS:  () GS:88085f5c() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: a01979de CR3: 01a0c000 CR4: 001406e0
Stack:
 812349b1 880852887cb8 880852887d10 88085f5cd6c2
 880852800a80 a01979de 880852800a84 0010
 88085bb28bd8 880852887d38 812354f0 880852887d08
Call Trace:
 [] ? __xlate_proc_name+0x71/0xd0
 [] remove_proc_entry+0x40/0x180
 [] ? _raw_spin_lock_irqsave+0x41/0x60
 [] ? destruct_tty_driver+0x60/0xe0
 [] proc_tty_unregister_driver+0x28/0x40
 [] destruct_tty_driver+0x88/0xe0
 [] tty_driver_kref_put+0x1d/0x20
 [] release_one_tty+0x5a/0xd0
 [] process_one_work+0x139/0x420
 [] worker_thread+0x121/0x450
 [] ? process_scheduled_works+0x40/0x40
 [] kthread+0xec/0x110
 [] ? tg_rt_schedulable+0x210/0x220
 [] ? kthread_freezable_should_stop+0x80/0x80
 [] ret_from_fork+0x42/0x70
 [] ? kthread_freezable_should_stop+0x80/0x80

Signed-off-by: nixiaoming 
---
 fs/proc/proc_tty.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/proc/proc_tty.c b/fs/proc/proc_tty.c
index 901bd06..20e2c18 100644
--- a/fs/proc/proc_tty.c
+++ b/fs/proc/proc_tty.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include "internal.h"
 
 /*
  * The /proc/tty directory inodes...
@@ -164,7 +165,7 @@ void proc_tty_unregister_driver(struct tty_driver *driver)
if (!ent)
return;

-   remove_proc_entry(driver->driver_name, proc_tty_driver);
+   remove_proc_entry(ent->name, proc_tty_driver);

driver->proc_entry = NULL;
 }
-- 
2.11.0.1

[PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook

[nixiaoming]

From: l00219569 <lisi...@huawei.com>

If fanout_add is preempted after running po-> fanout = match
and before running __fanout_link,
it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink

so, we need add mutex_lock(_mutex) to __unregister_prot_hook
or add spin_lock(>bind_lock) before po-> fanout = match

this is a patch for add po->bind_lock in fanout_add

test on linux 4.1.12:
./trinity -c setsockopt -C 2 -X &

BUG: failure at net/packet/af_packet.c:1414/__fanout_unlink()!
Kernel panic - not syncing: BUG!
CPU: 2 PID: 2271 Comm: trinity-c0 Tainted: GW  O4.1.12 #1
Hardware name: Hisilicon PhosphorHi1382 FPGA (DT)
Call trace:
[] dump_backtrace+0x0/0xf8
[] show_stack+0x20/0x28
[] dump_stack+0xac/0xe4
[] panic+0xf8/0x268
[] __unregister_prot_hook+0xa0/0x144
[] packet_set_ring+0x280/0x5b4
[] packet_setsockopt+0x320/0x950
[] SyS_setsockopt+0xa4/0xd4

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
Tested-by: wudesheng <dede...@huawei.com>
---
 net/packet/af_packet.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 54a18a8..7a52a3b 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1446,12 +1446,16 @@ static int fanout_add(struct sock *sk, u16 id, u16 
type_flags)
default:
return -EINVAL;
}
-
-   if (!po->running)
+   spin_lock(>bind_lock);
+   if (!po->running) {
+   spin_unlock(>bind_lock);
return -EINVAL;
+   }
 
-   if (po->fanout)
+   if (po->fanout) {
+   spin_unlock(>bind_lock);
return -EALREADY;
+   }
 
mutex_lock(_mutex);
match = NULL;
@@ -1501,6 +1505,7 @@ static int fanout_add(struct sock *sk, u16 id, u16 
type_flags)
}
 out:
mutex_unlock(_mutex);
+   spin_unlock(>bind_lock);
return err;
 }
 
-- 
2.10.1

[PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook

[nixiaoming]

From: l00219569 

If fanout_add is preempted after running po-> fanout = match
and before running __fanout_link,
it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink

so, we need add mutex_lock(_mutex) to __unregister_prot_hook
or add spin_lock(>bind_lock) before po-> fanout = match

this is a patch for add po->bind_lock in fanout_add

test on linux 4.1.12:
./trinity -c setsockopt -C 2 -X &

BUG: failure at net/packet/af_packet.c:1414/__fanout_unlink()!
Kernel panic - not syncing: BUG!
CPU: 2 PID: 2271 Comm: trinity-c0 Tainted: GW  O4.1.12 #1
Hardware name: Hisilicon PhosphorHi1382 FPGA (DT)
Call trace:
[] dump_backtrace+0x0/0xf8
[] show_stack+0x20/0x28
[] dump_stack+0xac/0xe4
[] panic+0xf8/0x268
[] __unregister_prot_hook+0xa0/0x144
[] packet_set_ring+0x280/0x5b4
[] packet_setsockopt+0x320/0x950
[] SyS_setsockopt+0xa4/0xd4

Signed-off-by: nixiaoming 
Tested-by: wudesheng 
---
 net/packet/af_packet.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 54a18a8..7a52a3b 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1446,12 +1446,16 @@ static int fanout_add(struct sock *sk, u16 id, u16 
type_flags)
default:
return -EINVAL;
}
-
-   if (!po->running)
+   spin_lock(>bind_lock);
+   if (!po->running) {
+   spin_unlock(>bind_lock);
return -EINVAL;
+   }
 
-   if (po->fanout)
+   if (po->fanout) {
+   spin_unlock(>bind_lock);
return -EALREADY;
+   }
 
mutex_lock(_mutex);
match = NULL;
@@ -1501,6 +1505,7 @@ static int fanout_add(struct sock *sk, u16 id, u16 
type_flags)
}
 out:
mutex_unlock(_mutex);
+   spin_unlock(>bind_lock);
return err;
 }
 
-- 
2.10.1

[PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook

[nixiaoming]

If fanout_add is preempted after running po-> fanout = match
and before running __fanout_link,
it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink

so, we need add mutex_lock(_mutex) to __unregister_prot_hook
or add spin_lock(>bind_lock) before po-> fanout = match

test on linux 4.1.12:
./trinity -c setsockopt -C 2 -X &

BUG: failure at net/packet/af_packet.c:1414/__fanout_unlink()!
Kernel panic - not syncing: BUG!
CPU: 2 PID: 2271 Comm: trinity-c0 Tainted: GW  O4.1.12 #1
Hardware name: Hisilicon PhosphorHi1382 FPGA (DT)
Call trace:
[] dump_backtrace+0x0/0xf8
[] show_stack+0x20/0x28
[] dump_stack+0xac/0xe4
[] panic+0xf8/0x268
[] __unregister_prot_hook+0xa0/0x144
[] packet_set_ring+0x280/0x5b4
[] packet_setsockopt+0x320/0x950
[] SyS_setsockopt+0xa4/0xd4

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
Tested-by: wudesheng <dede...@huawei.com>
---
 net/packet/af_packet.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 008a45c..0300146 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -365,10 +365,12 @@ static void __unregister_prot_hook(struct sock *sk, bool 
sync)
 
po->running = 0;
 
+   mutex_lock(_mutex);
if (po->fanout)
__fanout_unlink(sk, po);
else
__dev_remove_pack(>prot_hook);
+   mutex_unlock(_mutex);
 
__sock_put(sk);
 
-- 
2.11.0.1

[PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook

[nixiaoming]

If fanout_add is preempted after running po-> fanout = match
and before running __fanout_link,
it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink

so, we need add mutex_lock(_mutex) to __unregister_prot_hook
or add spin_lock(>bind_lock) before po-> fanout = match

test on linux 4.1.12:
./trinity -c setsockopt -C 2 -X &

BUG: failure at net/packet/af_packet.c:1414/__fanout_unlink()!
Kernel panic - not syncing: BUG!
CPU: 2 PID: 2271 Comm: trinity-c0 Tainted: GW  O4.1.12 #1
Hardware name: Hisilicon PhosphorHi1382 FPGA (DT)
Call trace:
[] dump_backtrace+0x0/0xf8
[] show_stack+0x20/0x28
[] dump_stack+0xac/0xe4
[] panic+0xf8/0x268
[] __unregister_prot_hook+0xa0/0x144
[] packet_set_ring+0x280/0x5b4
[] packet_setsockopt+0x320/0x950
[] SyS_setsockopt+0xa4/0xd4

Signed-off-by: nixiaoming 
Tested-by: wudesheng 
---
 net/packet/af_packet.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 008a45c..0300146 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -365,10 +365,12 @@ static void __unregister_prot_hook(struct sock *sk, bool 
sync)
 
po->running = 0;
 
+   mutex_lock(_mutex);
if (po->fanout)
__fanout_unlink(sk, po);
else
__dev_remove_pack(>prot_hook);
+   mutex_unlock(_mutex);
 
__sock_put(sk);
 
-- 
2.11.0.1

[PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook

[nixiaoming]

If fanout_add is preempted after running po-> fanout = match
and before running __fanout_link,
it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink

so, we need add mutex_lock(_mutex) to __unregister_prot_hook
or add spin_lock(>bind_lock) before po-> fanout = match

test on linux 4.1.42:
./trinity -c setsockopt -C 2 -X &

BUG: failure at net/packet/af_packet.c:1414/__fanout_unlink()!
Kernel panic - not syncing: BUG!
CPU: 2 PID: 2271 Comm: trinity-c0 Tainted: GW  O4.1.12 #1
Hardware name: Hisilicon PhosphorHi1382 FPGA (DT)
Call trace:
[] dump_backtrace+0x0/0xf8
[] show_stack+0x20/0x28
[] dump_stack+0xac/0xe4
[] panic+0xf8/0x268
[] __unregister_prot_hook+0xa0/0x144
[] packet_set_ring+0x280/0x5b4
[] packet_setsockopt+0x320/0x950
[] SyS_setsockopt+0xa4/0xd4

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
Tested-by: wudesheng <dede...@huawei.com>
---
 net/packet/af_packet.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 008a45c..0300146 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -365,10 +365,12 @@ static void __unregister_prot_hook(struct sock *sk, bool 
sync)
 
po->running = 0;
 
+   mutex_lock(_mutex);
if (po->fanout)
__fanout_unlink(sk, po);
else
__dev_remove_pack(>prot_hook);
+   mutex_unlock(_mutex);
 
__sock_put(sk);
 
-- 
2.11.0.1

[PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook

[nixiaoming]

If fanout_add is preempted after running po-> fanout = match
and before running __fanout_link,
it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink

so, we need add mutex_lock(_mutex) to __unregister_prot_hook
or add spin_lock(>bind_lock) before po-> fanout = match

test on linux 4.1.42:
./trinity -c setsockopt -C 2 -X &

BUG: failure at net/packet/af_packet.c:1414/__fanout_unlink()!
Kernel panic - not syncing: BUG!
CPU: 2 PID: 2271 Comm: trinity-c0 Tainted: GW  O4.1.12 #1
Hardware name: Hisilicon PhosphorHi1382 FPGA (DT)
Call trace:
[] dump_backtrace+0x0/0xf8
[] show_stack+0x20/0x28
[] dump_stack+0xac/0xe4
[] panic+0xf8/0x268
[] __unregister_prot_hook+0xa0/0x144
[] packet_set_ring+0x280/0x5b4
[] packet_setsockopt+0x320/0x950
[] SyS_setsockopt+0xa4/0xd4

Signed-off-by: nixiaoming 
Tested-by: wudesheng 
---
 net/packet/af_packet.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 008a45c..0300146 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -365,10 +365,12 @@ static void __unregister_prot_hook(struct sock *sk, bool 
sync)
 
po->running = 0;
 
+   mutex_lock(_mutex);
if (po->fanout)
__fanout_unlink(sk, po);
else
__dev_remove_pack(>prot_hook);
+   mutex_unlock(_mutex);
 
__sock_put(sk);
 
-- 
2.11.0.1

[PATCH] [RESEND] KVM:PPC:Book3s: fix memory leak in kvm_vm_ioctl_get_htab_fd

[nixiaoming]

ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
but no free when anon_inode_getfd return fail
so, add kfree(ctx) to fix memory leak

Signed-off-by: nixiaoming <nixiaom...@huawei.com>
Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>
---
 arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c 
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index b42812e..be3d08f 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -1940,6 +1940,7 @@ int kvm_vm_ioctl_get_htab_fd(struct kvm *kvm, struct 
kvm_get_htab_fd *ghf)
rwflag = (ghf->flags & KVM_GET_HTAB_WRITE) ? O_WRONLY : O_RDONLY;
ret = anon_inode_getfd("kvm-htab", _htab_fops, ctx, rwflag | 
O_CLOEXEC);
if (ret < 0) {
+   kfree(ctx);
kvm_put_kvm(kvm);
return ret;
}
-- 
2.11.0.1

[PATCH] [RESEND] KVM:PPC:Book3s: fix memory leak in kvm_vm_ioctl_get_htab_fd

[nixiaoming]

ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
but no free when anon_inode_getfd return fail
so, add kfree(ctx) to fix memory leak

Signed-off-by: nixiaoming 
Reviewed-by: Paolo Bonzini 
---
 arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c 
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index b42812e..be3d08f 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -1940,6 +1940,7 @@ int kvm_vm_ioctl_get_htab_fd(struct kvm *kvm, struct 
kvm_get_htab_fd *ghf)
rwflag = (ghf->flags & KVM_GET_HTAB_WRITE) ? O_WRONLY : O_RDONLY;
ret = anon_inode_getfd("kvm-htab", _htab_fops, ctx, rwflag | 
O_CLOEXEC);
if (ret < 0) {
+   kfree(ctx);
kvm_put_kvm(kvm);
return ret;
}
-- 
2.11.0.1