Received: by 2002:a25:f815:0:0:0:0:0 with SMTP id u21csp3007013ybd; Fri, 28 Jun 2019 01:02:59 -0700 (PDT) X-Google-Smtp-Source: APXvYqxFTL6hf+kJ7COjBS5EuqCJLUOKPTkKaGAPhTRRXJBDfex8kFdUWvCjya+NlwcCNmS6pxO5 X-Received: by 2002:a17:902:b43:: with SMTP id 61mr10162780plq.322.1561708979645; Fri, 28 Jun 2019 01:02:59 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1561708979; cv=none; d=google.com; s=arc-20160816; b=HMvIYzsBRsw9cFJ9GxbQhepLKWJGFyI2ha18cLLccpepqr0S+G0p5j2DGxjnJFUi73 5MZPmJhhpOrbri7jZqJWsHW3Mrj34DrLp+RN25DwxH8jFNdJTY33C/3P56XrBKSmhAEd P6dpGknfhclHv5d6JeCb91V3XL/Cjn2H3UStMnjRnS0sqUZxzf589SUvhRJWo9T59ixd pkrS6Wky26oPhdMLeKUaGnl26zPRDBCe0N0KjrhZ5Wvw+HYK1pF3Zu8BLoQhXXx0/gG4 ZdlI2nZ18hF0taaf2fsHR3EXIDoiZzbh4xJdkJyNTahJvIgtL1KWWPn27KU++0xt2owh DdiA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=WZq6BCzx2oAYKBXZwpH6LRexuDJ0/444+0e0Mc/hDbY=; b=YGAIyV6xULz6f4Q7FU/L/k8E/SiGmMKwCb0HV95IavPC2w81Oc/BrOZOtbP9g3OVbx WA9/gy0yxJttwMsRQ6TwZKSPtHEqZ0aa3eCi8doCLlBjTl69iARvZQt7ndFRuG+cf4bk toFXU40A+Oqsz7XEBQwpmeKFZILhJXRAz+wUNdDR4eduoQSmLDWYk8Y1iZCJUDuhm6UT seqU9Any0iunTuCJH16meBglvU3M4ME9KWfOEl6lQOoZfoAk7DzHeHjCrhxImYeziw7L OPrWsHgYDwrVgo2L2u1SjRW43IuaLVZhe/FIW5eV+oaUS1MBJi/Yf5bckppadKXDyi6x R3hg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=Rd9lRP9z; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id w8si1392748pgr.258.2019.06.28.01.02.41; Fri, 28 Jun 2019 01:02:59 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=Rd9lRP9z; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726557AbfF1ICH (ORCPT + 99 others); Fri, 28 Jun 2019 04:02:07 -0400 Received: from mail-pl1-f195.google.com ([209.85.214.195]:34278 "EHLO mail-pl1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726514AbfF1ICG (ORCPT ); Fri, 28 Jun 2019 04:02:06 -0400 Received: by mail-pl1-f195.google.com with SMTP id i2so2802237plt.1 for ; Fri, 28 Jun 2019 01:02:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=WZq6BCzx2oAYKBXZwpH6LRexuDJ0/444+0e0Mc/hDbY=; b=Rd9lRP9zAd/kbl3ZE7qYqE71uAPpqI08y9O8Z8POPjewUXfV5BkcxB5GlkY7Lz2EvR L0vi5OB036U4FKFCgIDZv8F3y6lztLOxmk0kwy/NUBPaZkq+ECaVF1Jt2lfubLgAj49H Uk6C/xmHOoJyYfOl2AD99Cc4Ji0bJcI5iEK/IOzTvY7wn8alGsOUKlv4FIhRP3sRGF55 fIMKwrwTaQtuY+hOsydah24wbl/mQVSW6sltZZBybHwghcYUR14MnHCnTHXmgdqPSQtw agAI8RdzSUB0Onytip3sOEuouLaTLd1x4D+UiNdsy+ktLsNeIwWlm9Q+91kPa3V5g02W Z4fA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=WZq6BCzx2oAYKBXZwpH6LRexuDJ0/444+0e0Mc/hDbY=; b=FqoIf4seZOMeTFuqhjN2s5khvYb4868OAawF8i7mGZnImKx55I1f9wpviQcjx4NPUh 8VcPvzB7ONbAh4zubSEx4KMiJN6mBN09NFtTcHtgUGkRbkxjCKWue4mFwW3zfZEAZmqZ 0zM5e+461pSjNCUzba1r7gZ8fIzZLDLvilqG9kKfUQniXkQWhgfyXdK1AHF5K4RjYEys jyYZLKrQYG6vAmjOqft+PerEsXN5fBfgTBLjlfXJKewsj14xx9fFw+Gudmqjx8cq5sif R+PqT3WQ7o1ibOHg/yOzZJDuw/H2OkDHjicZaLBsSY52lQdp7aqQrraYC0nZbWre1k0E 5V1w== X-Gm-Message-State: APjAAAXkZkv9RyxoNmq1gazhmUvlL7nI8fzMEjgYynMqA9xh1mKT/V8W b+5zf1p00AnRnXXJbHHmtivc4UHdpOPZCetuWWuULQ== X-Received: by 2002:a17:902:2006:: with SMTP id n6mr10173684pla.232.1561708925220; Fri, 28 Jun 2019 01:02:05 -0700 (PDT) MIME-Version: 1.0 References: <20190617082613.109131-1-brendanhiggins@google.com> <20190617082613.109131-18-brendanhiggins@google.com> <20190626021744.GU19023@42.do-not-panic.com> <20190627061021.GE19023@42.do-not-panic.com> In-Reply-To: <20190627061021.GE19023@42.do-not-panic.com> From: Brendan Higgins Date: Fri, 28 Jun 2019 01:01:54 -0700 Message-ID: Subject: Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec() To: Luis Chamberlain Cc: Iurii Zaikin , linux-api@vger.kernel.org, "Michael Kerrisk (man-pages)" , Frank Rowand , Greg KH , Josh Poimboeuf , Kees Cook , Kieran Bingham , Peter Zijlstra , Rob Herring , Stephen Boyd , shuah , "Theodore Ts'o" , Masahiro Yamada , devicetree , dri-devel , kunit-dev@googlegroups.com, "open list:DOCUMENTATION" , linux-fsdevel@vger.kernel.org, linux-kbuild , Linux Kernel Mailing List , "open list:KERNEL SELFTEST FRAMEWORK" , linux-nvdimm , linux-um@lists.infradead.org, Sasha Levin , "Bird, Timothy" , Amir Goldstein , Dan Carpenter , Daniel Vetter , Jeff Dike , Joel Stanley , Julia Lawall , Kevin Hilman , Knut Omang , Logan Gunthorpe , Michael Ellerman , Petr Mladek , Randy Dunlap , Richard Weinberger , David Rientjes , Steven Rostedt , wfg@linux.intel.com Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jun 26, 2019 at 11:10 PM Luis Chamberlain wrote: > > On Wed, Jun 26, 2019 at 09:07:43PM -0700, Iurii Zaikin wrote: > > On Tue, Jun 25, 2019 at 7:17 PM Luis Chamberlain wrote: > > > > +static void sysctl_test_dointvec_table_maxlen_unset(struct kunit *test) > > > > +{ > > > > + struct ctl_table table = { > > > > + .procname = "foo", > > > > + .data = &test_data.int_0001, > > > > + .maxlen = 0, > > > > + .mode = 0644, > > > > + .proc_handler = proc_dointvec, > > > > + .extra1 = &i_zero, > > > > + .extra2 = &i_one_hundred, > > > > + }; > > > > + void *buffer = kunit_kzalloc(test, sizeof(int), GFP_USER); > > > > + size_t len; > > > > + loff_t pos; > > > > + > > > > + len = 1234; > > > > + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 0, buffer, &len, &pos)); > > > > + KUNIT_EXPECT_EQ(test, (size_t)0, len); > > > > + len = 1234; > > > > + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 1, buffer, &len, &pos)); > > > > + KUNIT_EXPECT_EQ(test, (size_t)0, len); > > > > +} > > > > > > In a way this is also testing for general kernel API changes. This is and the > > > last one were good examples. So this is not just testing functionality > > > here. There is no wrong or write answer if 0 or -EINVAL was returned > > > other than the fact that we have been doing this for years. > > > > > > Its a perhaps small but important difference for some of these tests. I > > > *do* think its worth clarifying through documentation which ones are > > > testing for API consistency Vs proper correctness. > > > > You make a good point that the test codifies the existing behavior of > > the function in lieu of formal documentation. However, the test cases > > were derived from examining the source code of the function under test > > and attempting to cover all branches. The assertions were added only > > for the values that appeared to be set deliberately in the > > implementation. And it makes sense to me to test that the code does > > exactly what the implementation author intended. > > I'm not arguing against adding them. I'm suggesting that it is different > to test for API than for correctness of intended functionality, and > it would be wise to make it clear which test cases are for API and which > for correctness. I see later on that some of the API stuff you are talking about is public APIs from the standpoint of user (outside of LInux) visible. To be clear, is that what you mean by public APIs throughout, or would you distinguish between correctness tests, internal API tests, and external API tests? > This will come up later for other kunit tests and it would be great > to set precendent so that other kunit tests can follow similar > practices to ensure its clear what is API realted Vs correctness of > intended functionality. > > In fact, I'm not yet sure if its possible to test public kernel API to > userspace with kunit, but if it is possible... well, that could make > linux-api folks happy as they could enable us to codify interpreation of > what is expected into kunit test cases, and we'd ensure that the > codified interpretation is not only documented in man pages but also > through formal kunit test cases. > > A regression in linux-api then could be formalized through a proper > kunit tests case. And if an API evolves, it would force developers to > update the respective kunit which codifies that contract. Yep, I think that is long term hope. Some of the file system interface stuff that requires a filesystem to be mounted somewhere might get a little weird/difficult, but I suspect we should be able to do it eventually. I mean it's all just C code right? Should mostly boil down to someone figuring out how to do it the first time. > > > > +static void sysctl_test_dointvec_single_less_int_min(struct kunit *test) > > > > +{ > > > > + struct ctl_table table = { > > > > + .procname = "foo", > > > > + .data = &test_data.int_0001, > > > > + .maxlen = sizeof(int), > > > > + .mode = 0644, > > > > + .proc_handler = proc_dointvec, > > > > + .extra1 = &i_zero, > > > > + .extra2 = &i_one_hundred, > > > > + }; > > > > + char input[32]; > > > > + size_t len = sizeof(input) - 1; > > > > + loff_t pos = 0; > > > > + unsigned long abs_of_less_than_min = (unsigned long)INT_MAX > > > > + - (INT_MAX + INT_MIN) + 1; > > > > + > > > > + KUNIT_EXPECT_LT(test, > > > > + (size_t)snprintf(input, sizeof(input), "-%lu", > > > > + abs_of_less_than_min), > > > > + sizeof(input)); > > > > + > > > > + table.data = kunit_kzalloc(test, sizeof(int), GFP_USER); > > > > + KUNIT_EXPECT_EQ(test, -EINVAL, > > > > + proc_dointvec(&table, 1, input, &len, &pos)); > > > > + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len); > > > > + KUNIT_EXPECT_EQ(test, 0, ((int *)table.data)[0]); > > > > +} > > > > > > API test. > > > > > Not sure why. > > Because you are codifying that we *definitely* return -EINVAL on > overlow. Some parts of the kernel return -ERANGE for overflows for > instance. > > It would be a generic test for overflow if it would just test > for any error. > > It is a fine and good test to keep. All these tests are good to keep. > > > I believe there has been a real bug with int overflow in > > proc_dointvec. > > Covering it with test seems like a good idea. > > Oh definitely. > > > > > +static void sysctl_test_dointvec_single_greater_int_max(struct kunit *test) > > > > +{ > > > > + struct ctl_table table = { > > > > + .procname = "foo", > > > > + .data = &test_data.int_0001, > > > > + .maxlen = sizeof(int), > > > > + .mode = 0644, > > > > + .proc_handler = proc_dointvec, > > > > + .extra1 = &i_zero, > > > > + .extra2 = &i_one_hundred, > > > > + }; > > > > + char input[32]; > > > > + size_t len = sizeof(input) - 1; > > > > + loff_t pos = 0; > > > > + unsigned long greater_than_max = (unsigned long)INT_MAX + 1; > > > > + > > > > + KUNIT_EXPECT_GT(test, greater_than_max, (unsigned long)INT_MAX); > > > > + KUNIT_EXPECT_LT(test, (size_t)snprintf(input, sizeof(input), "%lu", > > > > + greater_than_max), > > > > + sizeof(input)); > > > > + table.data = kunit_kzalloc(test, sizeof(int), GFP_USER); > > > > + KUNIT_EXPECT_EQ(test, -EINVAL, > > > > + proc_dointvec(&table, 1, input, &len, &pos)); > > > > + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len); > > > > + KUNIT_EXPECT_EQ(test, 0, ((int *)table.data)[0]); > > > > +} > > > > + > > > > > > API test. > > > > > > > +static struct kunit_case sysctl_test_cases[] = { > > > > + KUNIT_CASE(sysctl_test_dointvec_null_tbl_data), > > > > + KUNIT_CASE(sysctl_test_dointvec_table_maxlen_unset), > > > > + KUNIT_CASE(sysctl_test_dointvec_table_len_is_zero), > > > > + KUNIT_CASE(sysctl_test_dointvec_table_read_but_position_set), > > > > + KUNIT_CASE(sysctl_test_dointvec_happy_single_positive), > > > > + KUNIT_CASE(sysctl_test_dointvec_happy_single_negative), > > > > + KUNIT_CASE(sysctl_test_dointvec_single_less_int_min), > > > > + KUNIT_CASE(sysctl_test_dointvec_single_greater_int_max), > > > > + {} > > > > +}; > > > > > > Oh all are API tests.. perhaps then just rename then > > > sysctl_test_cases to sysctl_api_test_cases. > > > > > > Would be good to add at least *two* other tests cases for this > > > example, one which does a valid read and one which does a valid write. > > Added valid reads. There already are 2 valid writes. > > Thanks. > > > > If that is done either we add another kunit test module for correctness > > > or just extend the above and use prefix / postfixes on the functions > > > to distinguish between API / correctness somehow. > > > > > > > + > > > > +static struct kunit_module sysctl_test_module = { > > > > + .name = "sysctl_test", > > > > + .test_cases = sysctl_test_cases, > > > > +}; > > > > + > > > > +module_test(sysctl_test_module); > > > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > > > > index cbdfae3798965..389b8986f5b77 100644 > > > > --- a/lib/Kconfig.debug > > > > +++ b/lib/Kconfig.debug > > > > @@ -1939,6 +1939,16 @@ config TEST_SYSCTL > > > > > > > > If unsure, say N. > > > > > > > > +config SYSCTL_KUNIT_TEST > > > > + bool "KUnit test for sysctl" > > > > + depends on KUNIT > > > > + help > > > > + This builds the proc sysctl unit test, which runs on boot. For more > > > > + information on KUnit and unit tests in general please refer to the > > > > + KUnit documentation in Documentation/dev-tools/kunit/. > > > > > > A little more description here would help. It is testing for API and > > > hopefully also correctness (if extended with those two examples I > > > mentioned). > > > > > Added "Tests the API contract and implementation correctness of sysctl." > > Yes, much clearer, thanks! Cheers!