Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1631720 > unrolled thread

Boot regression caused by kauditd

Back to article view | Back to linux.kernel

Contents

  Boot regression caused by kauditd Cong Wang <xiyou.wangcong@gmail.com> - 2017-04-26 22:50 +0200
    Re: Boot regression caused by kauditd Paul Moore <paul@paul-moore.com> - 2017-04-26 23:30 +0200
      Re: Boot regression caused by kauditd Cong Wang <xiyou.wangcong@gmail.com> - 2017-04-27 07:10 +0200
      Re: Boot regression caused by kauditd Cong Wang <xiyou.wangcong@gmail.com> - 2017-04-27 22:40 +0200
        Re: Boot regression caused by kauditd Cong Wang <xiyou.wangcong@gmail.com> - 2017-04-27 23:40 +0200
          Re: Boot regression caused by kauditd Cong Wang <xiyou.wangcong@gmail.com> - 2017-04-27 23:50 +0200
            Re: Boot regression caused by kauditd Paul Moore <paul@paul-moore.com> - 2017-04-28 00:40 +0200
              Re: Boot regression caused by kauditd Cong Wang <xiyou.wangcong@gmail.com> - 2017-04-28 01:50 +0200
                Re: Boot regression caused by kauditd Paul Moore <paul@paul-moore.com> - 2017-04-28 02:50 +0200
                  Re: Boot regression caused by kauditd Paul Moore <paul@paul-moore.com> - 2017-04-28 17:40 +0200
                    Re: Boot regression caused by kauditd Cong Wang <xiyou.wangcong@gmail.com> - 2017-04-28 18:20 +0200
                      Re: Boot regression caused by kauditd Paul Moore <paul@paul-moore.com> - 2017-04-28 18:30 +0200
                        Re: Boot regression caused by kauditd Cong Wang <xiyou.wangcong@gmail.com> - 2017-05-02 08:00 +0200

#1631720 — Boot regression caused by kauditd

Hi,

I don't know if anyone else already reported this, but I randomly get
the following kernel crash during boot (Fedora 18, I know it's pretty
old!!):

[   20.940715] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000004
[   20.942122] IP: get_net+0x7/0xd
[   20.942739] PGD 0
[   20.942741]
[   20.943370] Oops: 0002 [#1] SMP
[   20.943910] CPU: 0 PID: 56 Comm: kauditd Not tainted 4.11.0-rc7+ #499
[   20.945056] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.8.2-20150714_191134- 04/01/2014
[   20.946675] task: ffff8800783f8040 task.stack: ffffc90000660000
[   20.947687] RIP: 0010:get_net+0x7/0xd
[   20.948310] RSP: 0018:ffffc90000663e98 EFLAGS: 00010202
[   20.949409] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 9dd28c6188bbd6db
[   20.951093] RDX: ffffffff8111556a RSI: 0000000057539c73 RDI: 0000000000000000
[   20.952382] RBP: ffffc90000663e98 R08: 0000000000000000 R09: 0000000000000000
[   20.953658] R10: ffffc90000663e20 R11: 0000000000000000 R12: 0000000000000000
[   20.954946] R13: 0000000000000000 R14: ffff8800783f8040 R15: ffff8800783f8040
[   20.956144] FS:  0000000000000000(0000) GS:ffff88007d200000(0000)
knlGS:0000000000000000
[   20.957628] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   20.958672] CR2: 0000000000000004 CR3: 0000000062ea3000 CR4: 00000000000406f0
[   20.959943] Call Trace:
[   20.960380]  kauditd_thread+0x60/0x1dc
[   20.961027]  ? signal_pending_state+0x2f/0x2f
[   20.961771]  ? auditd_reset+0x5f/0x5f
[   20.962407]  kthread+0x107/0x10f
[   20.962963]  ? __list_del_entry+0x22/0x22
[   20.963657]  ret_from_fork+0x2e/0x40
[   20.964267] Code: 00 00 5b 41 5c 5d c3 31 c0 83 bf 84 00 00 00 00
55 48 89 e5 75 0c 8b 87 cc 00 00 00 2b 87 c8 00 00 00 5d c3 55 48 89
f8 48 89 e5 <f0> ff 47 04 5d c3 f0 ff 4f 04 74 01 c3 55 48 89 e5 e8 68
6b 7a
[   20.967651] RIP: get_net+0x7/0xd RSP: ffffc90000663e98
[   20.968526] CR2: 0000000000000004
[   20.969094] ---[ end trace aab4cbd6ae264eab ]---
[   20.969886] Kernel panic - not syncing: Fatal exception
[   20.971312] Kernel Offset: disabled
[   20.971928] ---[ end Kernel panic - not syncing: Fatal exception


I guess commit 264d509637d95f9404e introduced this because I never see
this crash before pulling net-next today, but given the fact I only
saw this once so far, I can't just revert it to verify if it is the
case... It is probably some race condition.

Please let me know if you need any other information.

Thanks!

[toc] | [next] | [standalone]

#1631734

On Wed, Apr 26, 2017 at 4:48 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> Hi,
>
> I don't know if anyone else already reported this, but I randomly get
> the following kernel crash during boot (Fedora 18, I know it's pretty
> old!!):
>
> [   20.940715] BUG: unable to handle kernel NULL pointer dereference
> at 0000000000000004
> [   20.942122] IP: get_net+0x7/0xd
> [   20.942739] PGD 0
> [   20.942741]
> [   20.943370] Oops: 0002 [#1] SMP
> [   20.943910] CPU: 0 PID: 56 Comm: kauditd Not tainted 4.11.0-rc7+ #499
> [   20.945056] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS 1.8.2-20150714_191134- 04/01/2014
> [   20.946675] task: ffff8800783f8040 task.stack: ffffc90000660000
> [   20.947687] RIP: 0010:get_net+0x7/0xd
> [   20.948310] RSP: 0018:ffffc90000663e98 EFLAGS: 00010202
> [   20.949409] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 9dd28c6188bbd6db
> [   20.951093] RDX: ffffffff8111556a RSI: 0000000057539c73 RDI: 0000000000000000
> [   20.952382] RBP: ffffc90000663e98 R08: 0000000000000000 R09: 0000000000000000
> [   20.953658] R10: ffffc90000663e20 R11: 0000000000000000 R12: 0000000000000000
> [   20.954946] R13: 0000000000000000 R14: ffff8800783f8040 R15: ffff8800783f8040
> [   20.956144] FS:  0000000000000000(0000) GS:ffff88007d200000(0000)
> knlGS:0000000000000000
> [   20.957628] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   20.958672] CR2: 0000000000000004 CR3: 0000000062ea3000 CR4: 00000000000406f0
> [   20.959943] Call Trace:
> [   20.960380]  kauditd_thread+0x60/0x1dc
> [   20.961027]  ? signal_pending_state+0x2f/0x2f
> [   20.961771]  ? auditd_reset+0x5f/0x5f
> [   20.962407]  kthread+0x107/0x10f
> [   20.962963]  ? __list_del_entry+0x22/0x22
> [   20.963657]  ret_from_fork+0x2e/0x40
> [   20.964267] Code: 00 00 5b 41 5c 5d c3 31 c0 83 bf 84 00 00 00 00
> 55 48 89 e5 75 0c 8b 87 cc 00 00 00 2b 87 c8 00 00 00 5d c3 55 48 89
> f8 48 89 e5 <f0> ff 47 04 5d c3 f0 ff 4f 04 74 01 c3 55 48 89 e5 e8 68
> 6b 7a
> [   20.967651] RIP: get_net+0x7/0xd RSP: ffffc90000663e98
> [   20.968526] CR2: 0000000000000004
> [   20.969094] ---[ end trace aab4cbd6ae264eab ]---
> [   20.969886] Kernel panic - not syncing: Fatal exception
> [   20.971312] Kernel Offset: disabled
> [   20.971928] ---[ end Kernel panic - not syncing: Fatal exception
>
>
> I guess commit 264d509637d95f9404e introduced this because I never see
> this crash before pulling net-next today, but given the fact I only
> saw this once so far, I can't just revert it to verify if it is the
> case... It is probably some race condition.
>
> Please let me know if you need any other information.

Hi,

Thanks for the report, this is the only one like it that I've seen.
I'm looking at the code in Linus' tree and I'm not seeing anything
obvious ... looking at the trace above it appears that the problem is
when get_net() goes to bump the refcount and the passed net pointer is
NULL; unless I'm missing something, the only way this would happen in
kauditd_thread() is if the auditd_conn.pid value is non-zero but the
auditd_conn.net pointer is NULL.

That shouldn't happen.

The only way I could see that even being possible is if the
sock_net(NETLINK_CB(skb).sk) call in audit_receive_msg() returned NULL
which in turned was passed as the net pointer (third parameter) in the
auditd_set() call.  Once again, I don't think this should ever be
possible?  Am I missing something?

I realize you aren't able to reproduce this reliably, but if you do,
any chance you try it with Linus' tree?  I'd like to see if we can
rule out the changes in net-next (my testing doesn't typically include
net-next patches).

-- 
paul moore
www.paul-moore.com

[toc] | [prev] | [next] | [standalone]

#1631860

On Wed, Apr 26, 2017 at 2:20 PM, Paul Moore <paul@paul-moore.com> wrote:
> Hi,
>
> Thanks for the report, this is the only one like it that I've seen.
> I'm looking at the code in Linus' tree and I'm not seeing anything
> obvious ... looking at the trace above it appears that the problem is
> when get_net() goes to bump the refcount and the passed net pointer is
> NULL; unless I'm missing something, the only way this would happen in
> kauditd_thread() is if the auditd_conn.pid value is non-zero but the
> auditd_conn.net pointer is NULL.
>
> That shouldn't happen.
>
> The only way I could see that even being possible is if the
> sock_net(NETLINK_CB(skb).sk) call in audit_receive_msg() returned NULL
> which in turned was passed as the net pointer (third parameter) in the
> auditd_set() call.  Once again, I don't think this should ever be
> possible?  Am I missing something?

I don't have time to look into it yet, I think I can take a look tomorrow.

>
> I realize you aren't able to reproduce this reliably, but if you do,
> any chance you try it with Linus' tree?  I'd like to see if we can
> rule out the changes in net-next (my testing doesn't typically include
> net-next patches).
>

Will do, I tried to boot for ~6 times today to reproduce it but just saw
it once.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1632368

On Wed, Apr 26, 2017 at 2:20 PM, Paul Moore <paul@paul-moore.com> wrote:
> Thanks for the report, this is the only one like it that I've seen.
> I'm looking at the code in Linus' tree and I'm not seeing anything
> obvious ... looking at the trace above it appears that the problem is
> when get_net() goes to bump the refcount and the passed net pointer is
> NULL; unless I'm missing something, the only way this would happen in
> kauditd_thread() is if the auditd_conn.pid value is non-zero but the
> auditd_conn.net pointer is NULL.
>
> That shouldn't happen.
>

Looking at the code that reads/writes the global auditd_conn,
I don't see how it even works with RCU+spinlock, RCU plays
with pointers and you have to make a copy as its name implies.
But it looks like you simply use RCU+spinlock as a traditional
rwlock, it doesn't work.

[toc] | [prev] | [next] | [standalone]

#1632384

[Multipart message — attachments visible in raw view] — view raw

On Thu, Apr 27, 2017 at 1:31 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, Apr 26, 2017 at 2:20 PM, Paul Moore <paul@paul-moore.com> wrote:
>> Thanks for the report, this is the only one like it that I've seen.
>> I'm looking at the code in Linus' tree and I'm not seeing anything
>> obvious ... looking at the trace above it appears that the problem is
>> when get_net() goes to bump the refcount and the passed net pointer is
>> NULL; unless I'm missing something, the only way this would happen in
>> kauditd_thread() is if the auditd_conn.pid value is non-zero but the
>> auditd_conn.net pointer is NULL.
>>
>> That shouldn't happen.
>>
>
> Looking at the code that reads/writes the global auditd_conn,
> I don't see how it even works with RCU+spinlock, RCU plays
> with pointers and you have to make a copy as its name implies.
> But it looks like you simply use RCU+spinlock as a traditional
> rwlock, it doesn't work.

The attached patch seems working for me, I tried to boot my
VM for 4 times, so far no crash or warning.

Please let me know if it looks reasonable to you.

[toc] | [prev] | [next] | [standalone]

#1632392

[Multipart message — attachments visible in raw view] — view raw

On Thu, Apr 27, 2017 at 2:35 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Apr 27, 2017 at 1:31 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Wed, Apr 26, 2017 at 2:20 PM, Paul Moore <paul@paul-moore.com> wrote:
>>> Thanks for the report, this is the only one like it that I've seen.
>>> I'm looking at the code in Linus' tree and I'm not seeing anything
>>> obvious ... looking at the trace above it appears that the problem is
>>> when get_net() goes to bump the refcount and the passed net pointer is
>>> NULL; unless I'm missing something, the only way this would happen in
>>> kauditd_thread() is if the auditd_conn.pid value is non-zero but the
>>> auditd_conn.net pointer is NULL.
>>>
>>> That shouldn't happen.
>>>
>>
>> Looking at the code that reads/writes the global auditd_conn,
>> I don't see how it even works with RCU+spinlock, RCU plays
>> with pointers and you have to make a copy as its name implies.
>> But it looks like you simply use RCU+spinlock as a traditional
>> rwlock, it doesn't work.
>
> The attached patch seems working for me, I tried to boot my
> VM for 4 times, so far no crash or warning.
>

Or even better, save a memory allocation for reset path...

[toc] | [prev] | [next] | [standalone]

#1632423

On Thu, Apr 27, 2017 at 5:45 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Apr 27, 2017 at 2:35 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Thu, Apr 27, 2017 at 1:31 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Wed, Apr 26, 2017 at 2:20 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>> Thanks for the report, this is the only one like it that I've seen.
>>>> I'm looking at the code in Linus' tree and I'm not seeing anything
>>>> obvious ... looking at the trace above it appears that the problem is
>>>> when get_net() goes to bump the refcount and the passed net pointer is
>>>> NULL; unless I'm missing something, the only way this would happen in
>>>> kauditd_thread() is if the auditd_conn.pid value is non-zero but the
>>>> auditd_conn.net pointer is NULL.
>>>>
>>>> That shouldn't happen.
>>>>
>>>
>>> Looking at the code that reads/writes the global auditd_conn,
>>> I don't see how it even works with RCU+spinlock, RCU plays
>>> with pointers and you have to make a copy as its name implies.
>>> But it looks like you simply use RCU+spinlock as a traditional
>>> rwlock, it doesn't work.
>>
>> The attached patch seems working for me, I tried to boot my
>> VM for 4 times, so far no crash or warning.
>>
>
> Or even better, save a memory allocation for reset path...

I need to step away from my laptop for the evening so I can't give
this a proper review until tomorrow (sending patches as attachments
makes it difficult to review), but on quick glance I did notice a few
small things I would like to see changed.  However, since there is no
normal commit description and sign-off, I'm guessing you sent these
out as a suggestion and not a proper patch submission, yes/no?  If
that's the case, I'll work up a proper fix tomorrow and share it with
you for comment/review, but if you were planning on sending a proper
patch let me know and I'll wait until I see something in my inbox from
you.

-- 
paul moore
www.paul-moore.com

[toc] | [prev] | [next] | [standalone]

#1632444

On Thu, Apr 27, 2017 at 3:38 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Apr 27, 2017 at 5:45 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Thu, Apr 27, 2017 at 2:35 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Thu, Apr 27, 2017 at 1:31 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>> On Wed, Apr 26, 2017 at 2:20 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>>> Thanks for the report, this is the only one like it that I've seen.
>>>>> I'm looking at the code in Linus' tree and I'm not seeing anything
>>>>> obvious ... looking at the trace above it appears that the problem is
>>>>> when get_net() goes to bump the refcount and the passed net pointer is
>>>>> NULL; unless I'm missing something, the only way this would happen in
>>>>> kauditd_thread() is if the auditd_conn.pid value is non-zero but the
>>>>> auditd_conn.net pointer is NULL.
>>>>>
>>>>> That shouldn't happen.
>>>>>
>>>>
>>>> Looking at the code that reads/writes the global auditd_conn,
>>>> I don't see how it even works with RCU+spinlock, RCU plays
>>>> with pointers and you have to make a copy as its name implies.
>>>> But it looks like you simply use RCU+spinlock as a traditional
>>>> rwlock, it doesn't work.
>>>
>>> The attached patch seems working for me, I tried to boot my
>>> VM for 4 times, so far no crash or warning.
>>>
>>
>> Or even better, save a memory allocation for reset path...
>
> I need to step away from my laptop for the evening so I can't give
> this a proper review until tomorrow (sending patches as attachments
> makes it difficult to review), but on quick glance I did notice a few
> small things I would like to see changed.  However, since there is no
> normal commit description and sign-off, I'm guessing you sent these
> out as a suggestion and not a proper patch submission, yes/no?  If
> that's the case, I'll work up a proper fix tomorrow and share it with
> you for comment/review, but if you were planning on sending a proper
> patch let me know and I'll wait until I see something in my inbox from
> you.

I want you to give it sanity check before I submit a formal one. ;)
If you don't reject it, I will send a formal one with description and SoB.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1632465

In that case please send a proper inline patch to the audit mailing list
and we'll review it.

Thanks.
--
paul moore
www.paul-moore.com



On April 27, 2017 7:41:45 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:

> On Thu, Apr 27, 2017 at 3:38 PM, Paul Moore <paul@paul-moore.com> wrote:
>> On Thu, Apr 27, 2017 at 5:45 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Thu, Apr 27, 2017 at 2:35 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>> On Thu, Apr 27, 2017 at 1:31 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>>> On Wed, Apr 26, 2017 at 2:20 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>>>> Thanks for the report, this is the only one like it that I've seen.
>>>>>> I'm looking at the code in Linus' tree and I'm not seeing anything
>>>>>> obvious ... looking at the trace above it appears that the problem is
>>>>>> when get_net() goes to bump the refcount and the passed net pointer is
>>>>>> NULL; unless I'm missing something, the only way this would happen in
>>>>>> kauditd_thread() is if the auditd_conn.pid value is non-zero but the
>>>>>> auditd_conn.net pointer is NULL.
>>>>>>
>>>>>> That shouldn't happen.
>>>>>>
>>>>>
>>>>> Looking at the code that reads/writes the global auditd_conn,
>>>>> I don't see how it even works with RCU+spinlock, RCU plays
>>>>> with pointers and you have to make a copy as its name implies.
>>>>> But it looks like you simply use RCU+spinlock as a traditional
>>>>> rwlock, it doesn't work.
>>>>
>>>> The attached patch seems working for me, I tried to boot my
>>>> VM for 4 times, so far no crash or warning.
>>>>
>>>
>>> Or even better, save a memory allocation for reset path...
>>
>> I need to step away from my laptop for the evening so I can't give
>> this a proper review until tomorrow (sending patches as attachments
>> makes it difficult to review), but on quick glance I did notice a few
>> small things I would like to see changed.  However, since there is no
>> normal commit description and sign-off, I'm guessing you sent these
>> out as a suggestion and not a proper patch submission, yes/no?  If
>> that's the case, I'll work up a proper fix tomorrow and share it with
>> you for comment/review, but if you were planning on sending a proper
>> patch let me know and I'll wait until I see something in my inbox from
>> you.
>
> I want you to give it sanity check before I submit a formal one. ;)
> If you don't reject it, I will send a formal one with description and SoB.
>
> Thanks.

[toc] | [prev] | [next] | [standalone]

#1632972

On Thu, Apr 27, 2017 at 8:47 PM, Paul Moore <paul@paul-moore.com> wrote:
> In that case please send a proper inline patch to the audit mailing list
> and we'll review it.
>
> Thanks.

Now that I'm back in front of a proper screen/keyboard I've been
looking over your patch and while you are very right in that the
current RCU usage is very wrong, there are quite a few things I would
like to see changed in your patch ... I'm working on something right
now, I'll post an RFC draft to the audit list and CC you once I get
this sorted out, expect something in a few hours.

Also, once you've had a look at this new patch, and assuming you are
okay with it, I'd like to add your sign-off to it.  This may not be
your patch exactly, but a significant portion of it is borrowed from
your patch yesterday.

> On April 27, 2017 7:41:45 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
>> On Thu, Apr 27, 2017 at 3:38 PM, Paul Moore <paul@paul-moore.com> wrote:
>>> On Thu, Apr 27, 2017 at 5:45 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>> On Thu, Apr 27, 2017 at 2:35 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>>> On Thu, Apr 27, 2017 at 1:31 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>>>> On Wed, Apr 26, 2017 at 2:20 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>>>>> Thanks for the report, this is the only one like it that I've seen.
>>>>>>> I'm looking at the code in Linus' tree and I'm not seeing anything
>>>>>>> obvious ... looking at the trace above it appears that the problem is
>>>>>>> when get_net() goes to bump the refcount and the passed net pointer is
>>>>>>> NULL; unless I'm missing something, the only way this would happen in
>>>>>>> kauditd_thread() is if the auditd_conn.pid value is non-zero but the
>>>>>>> auditd_conn.net pointer is NULL.
>>>>>>>
>>>>>>> That shouldn't happen.
>>>>>>>
>>>>>>
>>>>>> Looking at the code that reads/writes the global auditd_conn,
>>>>>> I don't see how it even works with RCU+spinlock, RCU plays
>>>>>> with pointers and you have to make a copy as its name implies.
>>>>>> But it looks like you simply use RCU+spinlock as a traditional
>>>>>> rwlock, it doesn't work.
>>>>>
>>>>> The attached patch seems working for me, I tried to boot my
>>>>> VM for 4 times, so far no crash or warning.
>>>>>
>>>>
>>>> Or even better, save a memory allocation for reset path...
>>>
>>> I need to step away from my laptop for the evening so I can't give
>>> this a proper review until tomorrow (sending patches as attachments
>>> makes it difficult to review), but on quick glance I did notice a few
>>> small things I would like to see changed.  However, since there is no
>>> normal commit description and sign-off, I'm guessing you sent these
>>> out as a suggestion and not a proper patch submission, yes/no?  If
>>> that's the case, I'll work up a proper fix tomorrow and share it with
>>> you for comment/review, but if you were planning on sending a proper
>>> patch let me know and I'll wait until I see something in my inbox from
>>> you.
>>
>> I want you to give it sanity check before I submit a formal one. ;)
>> If you don't reject it, I will send a formal one with description and SoB.
>>
>> Thanks.
>
>



-- 
paul moore
www.paul-moore.com

[toc] | [prev] | [next] | [standalone]

#1632996

On Fri, Apr 28, 2017 at 8:30 AM, Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Apr 27, 2017 at 8:47 PM, Paul Moore <paul@paul-moore.com> wrote:
>> In that case please send a proper inline patch to the audit mailing list
>> and we'll review it.
>>
>> Thanks.
>
> Now that I'm back in front of a proper screen/keyboard I've been
> looking over your patch and while you are very right in that the
> current RCU usage is very wrong, there are quite a few things I would
> like to see changed in your patch ... I'm working on something right
> now, I'll post an RFC draft to the audit list and CC you once I get
> this sorted out, expect something in a few hours.
>
> Also, once you've had a look at this new patch, and assuming you are
> okay with it, I'd like to add your sign-off to it.  This may not be
> your patch exactly, but a significant portion of it is borrowed from
> your patch yesterday.

So your review process is: if people's V1 patch is not perfect, you
will rewrite it by yourself?

But the normal review process is: people need to address feedback
and send V2, V3 etc..

That's too odd. Someday, no one will be willing to work on audit
patches except yourself.

No offense, just don't feel your review process is cooperative...

[toc] | [prev] | [next] | [standalone]

#1633012

On Fri, Apr 28, 2017 at 12:11 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Apr 28, 2017 at 8:30 AM, Paul Moore <paul@paul-moore.com> wrote:
>> On Thu, Apr 27, 2017 at 8:47 PM, Paul Moore <paul@paul-moore.com> wrote:
>>> In that case please send a proper inline patch to the audit mailing list
>>> and we'll review it.
>>>
>>> Thanks.
>>
>> Now that I'm back in front of a proper screen/keyboard I've been
>> looking over your patch and while you are very right in that the
>> current RCU usage is very wrong, there are quite a few things I would
>> like to see changed in your patch ... I'm working on something right
>> now, I'll post an RFC draft to the audit list and CC you once I get
>> this sorted out, expect something in a few hours.
>>
>> Also, once you've had a look at this new patch, and assuming you are
>> okay with it, I'd like to add your sign-off to it.  This may not be
>> your patch exactly, but a significant portion of it is borrowed from
>> your patch yesterday.
>
> So your review process is: if people's V1 patch is not perfect, you
> will rewrite it by yourself?

As I mentioned earlier I didn't get a chance to properly review your
patch yesterday for two important reasons: 1) it hit my inbox at the
end of my day and I simply didn't have time and 2) you sent it as an
attachment which makes it hard to review and provide feedback.  I took
a closer look at your patch this morning and noticed a number of
things that needed additional work as well as some merge/porting
things; considering I never saw a response from you on my last email
asking for an inline patch submission and taking into account where we
are in the merge window (I'd like to submit this fix during the v4.12
merge window) I went ahead and put together a patch based on your
prototype.

You'll see I just posted it and CC'd you (our emails probably crossed
paths), asking if it was okay to add your sign-off and give you
credit.  I'm only trying to speed up the process.  There is no malice
here, I actually thought I was helping you out ... I suppose the old
adage rings true: no good deed goes unpunished ;)

-- 
paul moore
www.paul-moore.com

[toc] | [prev] | [next] | [standalone]

#1634150

On Fri, Apr 28, 2017 at 9:26 AM, Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Apr 28, 2017 at 12:11 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Fri, Apr 28, 2017 at 8:30 AM, Paul Moore <paul@paul-moore.com> wrote:
>>> On Thu, Apr 27, 2017 at 8:47 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>> In that case please send a proper inline patch to the audit mailing list
>>>> and we'll review it.
>>>>
>>>> Thanks.
>>>
>>> Now that I'm back in front of a proper screen/keyboard I've been
>>> looking over your patch and while you are very right in that the
>>> current RCU usage is very wrong, there are quite a few things I would
>>> like to see changed in your patch ... I'm working on something right
>>> now, I'll post an RFC draft to the audit list and CC you once I get
>>> this sorted out, expect something in a few hours.
>>>
>>> Also, once you've had a look at this new patch, and assuming you are
>>> okay with it, I'd like to add your sign-off to it.  This may not be
>>> your patch exactly, but a significant portion of it is borrowed from
>>> your patch yesterday.
>>
>> So your review process is: if people's V1 patch is not perfect, you
>> will rewrite it by yourself?
>
> As I mentioned earlier I didn't get a chance to properly review your
> patch yesterday for two important reasons: 1) it hit my inbox at the
> end of my day and I simply didn't have time and 2) you sent it as an
> attachment which makes it hard to review and provide feedback.  I took
> a closer look at your patch this morning and noticed a number of
> things that needed additional work as well as some merge/porting
> things; considering I never saw a response from you on my last email
> asking for an inline patch submission and taking into account where we
> are in the merge window (I'd like to submit this fix during the v4.12
> merge window) I went ahead and put together a patch based on your
> prototype.
>
> You'll see I just posted it and CC'd you (our emails probably crossed
> paths), asking if it was okay to add your sign-off and give you
> credit.  I'm only trying to speed up the process.  There is no malice
> here, I actually thought I was helping you out ... I suppose the old
> adage rings true: no good deed goes unpunished ;)

Sure, it is of course faster since you are the maintainer, but this is
not how we work as a whole community. I am sure it could be very
offensive if you just rewrite other new people's patch only to speed
up.

Speed is one thing, collaboration is another, for the long term the
latter is much more important than the former for kernel community.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web