Date: Thu, 13 Dec 2012 06:00:01 GMT From: Garrett Cooper <yanegomi@gmail.com> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/174213: [fed] OFED sysfs/sysctl compat does not handle show/set attribute functions Message-ID: <201212130600.qBD601w5046021@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/174213; it has been noted by GNATS.
From: Garrett Cooper <yanegomi@gmail.com>
To: bug-followup@FreeBSD.org, accornehl@gmail.com
Cc:
Subject: Re: kern/174213: [fed] OFED sysfs/sysctl compat does not handle
show/set attribute functions
Date: Wed, 12 Dec 2012 21:50:05 -0800 (PST)
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--967339439-20611481-1355377809=:90705
Content-Type: TEXT/PLAIN; format=flowed; charset=US-ASCII
The attached patch fixes the issue and with the directions I
discussed on the mailing list, allowed me to flip between eth(ernet) mode
and ib mode.
Thanks!
-Garrett
From 53d4589061fe5c0dbcb3b58c3bcbb25fb212585a Mon Sep 17 00:00:00 2001
From: Garrett Cooper <yanegomi@gmail.com>
Date: Tue, 11 Dec 2012 22:10:00 -0800
Subject: [PATCH] Make the ofed sysctl<->sysfs handler more linux-like and
optimize it
Highlights:
The previous code was incorrectly assuming that sysfs and sysctls have a
1:1 relationship, but sysfs on Linux based on the documentation I've
found (and the Linux driver / example code I've peeked at) assumes
there's a trailing newline at the end of each value; so we need to
append a newline to the sysctl buffer on store and remove it on show in
order to be properly compatible between sysctl(9) and sysfs.
Also, the wrong value was being passed into SYSCTL_IN (PAGE_SIZE). It
should have been req->newlen.
Lowlights:
1. Many of the error conditions could have been simplified up-front
(dealing with checking the kobj object and whether or not buf was
NULL).
2. buf was being leaked if the kobj object was invalid.
3. The code now allows the user to store and show, store and not show,
or show and not store.
The issues highlighted address kern/174213.
Sponsored-by: EMC Corporation
Signed-off-by: Garrett Cooper <yanegomi@gmail.com>
---
sys/ofed/include/linux/sysfs.h | 53 ++++++++++++++++++++++++++++--------------
1 file changed, 35 insertions(+), 18 deletions(-)
diff --git a/sys/ofed/include/linux/sysfs.h b/sys/ofed/include/linux/sysfs.h
index 698f75e..1aff3c2 100644
--- a/sys/ofed/include/linux/sysfs.h
+++ b/sys/ofed/include/linux/sysfs.h
@@ -69,6 +69,10 @@ struct attribute_group {
* a constant string: point arg1 at it, arg2 is zero.
*/
+/*
+ * XXX: should support iteration of pages as sysfs supports paging
+ * in/paging out data in PAGE_SIZE chunks.
+ */
static inline int
sysctl_handle_attr(SYSCTL_HANDLER_ARGS)
{
@@ -76,37 +80,50 @@ sysctl_handle_attr(SYSCTL_HANDLER_ARGS)
struct attribute *attr;
const struct sysfs_ops *ops;
void *buf;
- int error;
ssize_t len;
+ int error = 0;
kobj = arg1;
attr = (struct attribute *)arg2;
- buf = (void *)get_zeroed_page(GFP_KERNEL);
- len = 1; /* Copy out a NULL byte at least. */
if (kobj->ktype == NULL || kobj->ktype->sysfs_ops == NULL)
return (ENODEV);
- ops = kobj->ktype->sysfs_ops;
+ buf = (void *)get_zeroed_page(GFP_KERNEL);
if (buf == NULL)
return (ENOMEM);
- if (ops->show) {
- len = ops->show(kobj, attr, buf);
- /*
- * It's valid not to have a 'show' so we just return 1 byte
- * of NULL.
- */
- if (len < 0) {
- error = -len;
- len = 1;
- if (error != EIO)
- goto out;
- }
+ ops = kobj->ktype->sysfs_ops;
+
+ /* Show */
+ if (ops->show == NULL)
+ goto store;
+ len = ops->show(kobj, attr, buf);
+ if (len < 0) {
+ error = -len;
+ len = 1;
+ if (error != EIO)
+ goto out;
}
+ /* It's valid not to have a 'show' so we just return NUL. */
+ else if (len == 0)
+ len = 1;
+ len -= 1;
+ ((char*)buf)[len] = '\0';
error = SYSCTL_OUT(req, buf, len);
- if (error || !req->newptr || ops->store == NULL)
+ if (error)
+ goto out;
+
+store:
+ /* Store */
+ if (!req->newptr || ops->store == NULL)
+ goto out;
+ if (req->newlen > PAGE_SIZE-2) { /* NUL terminated + '\n' */
+ error = EINVAL;
goto out;
- error = SYSCTL_IN(req, buf, PAGE_SIZE);
+ }
+ error = SYSCTL_IN(req, buf, req->newlen);
if (error)
goto out;
+ ((char*)buf)[req->newlen] = '\n';
+ ((char*)buf)[req->newlen+1] = '\0';
len = ops->store(kobj, attr, buf, req->newlen);
if (len < 0)
error = -len;
--
1.8.0.1
--967339439-20611481-1355377809=:90705
Content-Type: TEXT/PLAIN; charset=US-ASCII; name=0001-Make-the-ofed-sysctl-sysfs-handler-more-linux-like-a.patch
Content-Transfer-Encoding: BASE64
Content-ID: <alpine.BSF.2.00.1212122150050.90705@toaster.local>
Content-Description:
Content-Disposition: attachment; filename=0001-Make-the-ofed-sysctl-sysfs-handler-more-linux-like-a.patch
RnJvbSA1M2Q0NTg5MDYxZmU1YzBkYmNiM2I1OGMzYmNiYjI1ZmIyMTI1ODVh
IE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQ0KRnJvbTogR2FycmV0dCBDb29w
ZXIgPHlhbmVnb21pQGdtYWlsLmNvbT4NCkRhdGU6IFR1ZSwgMTEgRGVjIDIw
MTIgMjI6MTA6MDAgLTA4MDANClN1YmplY3Q6IFtQQVRDSF0gTWFrZSB0aGUg
b2ZlZCBzeXNjdGw8LT5zeXNmcyBoYW5kbGVyIG1vcmUgbGludXgtbGlrZSBh
bmQNCiBvcHRpbWl6ZSBpdA0KDQpIaWdobGlnaHRzOg0KDQpUaGUgcHJldmlv
dXMgY29kZSB3YXMgaW5jb3JyZWN0bHkgYXNzdW1pbmcgdGhhdCBzeXNmcyBh
bmQgc3lzY3RscyBoYXZlIGENCjE6MSByZWxhdGlvbnNoaXAsIGJ1dCBzeXNm
cyBvbiBMaW51eCBiYXNlZCBvbiB0aGUgZG9jdW1lbnRhdGlvbiBJJ3ZlDQpm
b3VuZCAoYW5kIHRoZSBMaW51eCBkcml2ZXIgLyBleGFtcGxlIGNvZGUgSSd2
ZSBwZWVrZWQgYXQpIGFzc3VtZXMNCnRoZXJlJ3MgYSB0cmFpbGluZyBuZXds
aW5lIGF0IHRoZSBlbmQgb2YgZWFjaCB2YWx1ZTsgc28gd2UgbmVlZCB0bw0K
YXBwZW5kIGEgbmV3bGluZSB0byB0aGUgc3lzY3RsIGJ1ZmZlciBvbiBzdG9y
ZSBhbmQgcmVtb3ZlIGl0IG9uIHNob3cgaW4NCm9yZGVyIHRvIGJlIHByb3Bl
cmx5IGNvbXBhdGlibGUgYmV0d2VlbiBzeXNjdGwoOSkgYW5kIHN5c2ZzLg0K
DQpBbHNvLCB0aGUgd3JvbmcgdmFsdWUgd2FzIGJlaW5nIHBhc3NlZCBpbnRv
IFNZU0NUTF9JTiAoUEFHRV9TSVpFKS4gSXQNCnNob3VsZCBoYXZlIGJlZW4g
cmVxLT5uZXdsZW4uDQoNCkxvd2xpZ2h0czoNCg0KMS4gTWFueSBvZiB0aGUg
ZXJyb3IgY29uZGl0aW9ucyBjb3VsZCBoYXZlIGJlZW4gc2ltcGxpZmllZCB1
cC1mcm9udA0KICAgKGRlYWxpbmcgd2l0aCBjaGVja2luZyB0aGUga29iaiBv
YmplY3QgYW5kIHdoZXRoZXIgb3Igbm90IGJ1ZiB3YXMNCiAgIE5VTEwpLg0K
Mi4gYnVmIHdhcyBiZWluZyBsZWFrZWQgaWYgdGhlIGtvYmogb2JqZWN0IHdh
cyBpbnZhbGlkLg0KMy4gVGhlIGNvZGUgbm93IGFsbG93cyB0aGUgdXNlciB0
byBzdG9yZSBhbmQgc2hvdywgc3RvcmUgYW5kIG5vdCBzaG93LA0KICAgb3Ig
c2hvdyBhbmQgbm90IHN0b3JlLg0KDQpUaGUgaXNzdWVzIGhpZ2hsaWdodGVk
IGFkZHJlc3Mga2Vybi8xNzQyMTMuDQoNClNwb25zb3JlZC1ieTogRU1DIENv
cnBvcmF0aW9uDQpTaWduZWQtb2ZmLWJ5OiBHYXJyZXR0IENvb3BlciA8eWFu
ZWdvbWlAZ21haWwuY29tPg0KLS0tDQogc3lzL29mZWQvaW5jbHVkZS9saW51
eC9zeXNmcy5oIHwgNTMgKysrKysrKysrKysrKysrKysrKysrKysrKysrKy0t
LS0tLS0tLS0tLS0tDQogMSBmaWxlIGNoYW5nZWQsIDM1IGluc2VydGlvbnMo
KyksIDE4IGRlbGV0aW9ucygtKQ0KDQpkaWZmIC0tZ2l0IGEvc3lzL29mZWQv
aW5jbHVkZS9saW51eC9zeXNmcy5oIGIvc3lzL29mZWQvaW5jbHVkZS9saW51
eC9zeXNmcy5oDQppbmRleCA2OThmNzVlLi4xYWZmM2MyIDEwMDY0NA0KLS0t
IGEvc3lzL29mZWQvaW5jbHVkZS9saW51eC9zeXNmcy5oDQorKysgYi9zeXMv
b2ZlZC9pbmNsdWRlL2xpbnV4L3N5c2ZzLmgNCkBAIC02OSw2ICs2OSwxMCBA
QCBzdHJ1Y3QgYXR0cmlidXRlX2dyb3VwIHsNCiAgKiAgICAgIGEgY29uc3Rh
bnQgc3RyaW5nOiAgcG9pbnQgYXJnMSBhdCBpdCwgYXJnMiBpcyB6ZXJvLg0K
ICAqLw0KIA0KKy8qDQorICogWFhYOiBzaG91bGQgc3VwcG9ydCBpdGVyYXRp
b24gb2YgcGFnZXMgYXMgc3lzZnMgc3VwcG9ydHMgcGFnaW5nDQorICogaW4v
cGFnaW5nIG91dCBkYXRhIGluIFBBR0VfU0laRSBjaHVua3MuDQorICovDQog
c3RhdGljIGlubGluZSBpbnQNCiBzeXNjdGxfaGFuZGxlX2F0dHIoU1lTQ1RM
X0hBTkRMRVJfQVJHUykNCiB7DQpAQCAtNzYsMzcgKzgwLDUwIEBAIHN5c2N0
bF9oYW5kbGVfYXR0cihTWVNDVExfSEFORExFUl9BUkdTKQ0KIAlzdHJ1Y3Qg
YXR0cmlidXRlICphdHRyOw0KIAljb25zdCBzdHJ1Y3Qgc3lzZnNfb3BzICpv
cHM7DQogCXZvaWQgKmJ1ZjsNCi0JaW50IGVycm9yOw0KIAlzc2l6ZV90IGxl
bjsNCisJaW50IGVycm9yID0gMDsNCiANCiAJa29iaiA9IGFyZzE7DQogCWF0
dHIgPSAoc3RydWN0IGF0dHJpYnV0ZSAqKWFyZzI7DQotCWJ1ZiA9ICh2b2lk
ICopZ2V0X3plcm9lZF9wYWdlKEdGUF9LRVJORUwpOw0KLQlsZW4gPSAxOwkv
KiBDb3B5IG91dCBhIE5VTEwgYnl0ZSBhdCBsZWFzdC4gKi8NCiAJaWYgKGtv
YmotPmt0eXBlID09IE5VTEwgfHwga29iai0+a3R5cGUtPnN5c2ZzX29wcyA9
PSBOVUxMKQ0KIAkJcmV0dXJuIChFTk9ERVYpOw0KLQlvcHMgPSBrb2JqLT5r
dHlwZS0+c3lzZnNfb3BzOw0KKwlidWYgPSAodm9pZCAqKWdldF96ZXJvZWRf
cGFnZShHRlBfS0VSTkVMKTsNCiAJaWYgKGJ1ZiA9PSBOVUxMKQ0KIAkJcmV0
dXJuIChFTk9NRU0pOw0KLQlpZiAob3BzLT5zaG93KSB7DQotCQlsZW4gPSBv
cHMtPnNob3coa29iaiwgYXR0ciwgYnVmKTsNCi0JCS8qDQotCQkgKiBJdCdz
IHZhbGlkIG5vdCB0byBoYXZlIGEgJ3Nob3cnIHNvIHdlIGp1c3QgcmV0dXJu
IDEgYnl0ZQ0KLQkJICogb2YgTlVMTC4NCi0JIAkgKi8NCi0JCWlmIChsZW4g
PCAwKSB7DQotCQkJZXJyb3IgPSAtbGVuOw0KLQkJCWxlbiA9IDE7DQotCQkJ
aWYgKGVycm9yICE9IEVJTykNCi0JCQkJZ290byBvdXQ7DQotCQl9DQorCW9w
cyA9IGtvYmotPmt0eXBlLT5zeXNmc19vcHM7DQorDQorCS8qIFNob3cgKi8N
CisJaWYgKG9wcy0+c2hvdyA9PSBOVUxMKQ0KKwkJZ290byBzdG9yZTsNCisJ
bGVuID0gb3BzLT5zaG93KGtvYmosIGF0dHIsIGJ1Zik7DQorCWlmIChsZW4g
PCAwKSB7DQorCQllcnJvciA9IC1sZW47DQorCQlsZW4gPSAxOw0KKwkJaWYg
KGVycm9yICE9IEVJTykNCisJCQlnb3RvIG91dDsNCiAJfQ0KKwkvKiBJdCdz
IHZhbGlkIG5vdCB0byBoYXZlIGEgJ3Nob3cnIHNvIHdlIGp1c3QgcmV0dXJu
IE5VTC4gKi8NCisJZWxzZSBpZiAobGVuID09IDApDQorCQlsZW4gPSAxOw0K
KwlsZW4gLT0gMTsNCisJKChjaGFyKilidWYpW2xlbl0gPSAnXDAnOw0KIAll
cnJvciA9IFNZU0NUTF9PVVQocmVxLCBidWYsIGxlbik7DQotCWlmIChlcnJv
ciB8fCAhcmVxLT5uZXdwdHIgfHwgb3BzLT5zdG9yZSA9PSBOVUxMKQ0KKwlp
ZiAoZXJyb3IpDQorCQlnb3RvIG91dDsNCisNCitzdG9yZToNCisJLyogU3Rv
cmUgKi8NCisJaWYgKCFyZXEtPm5ld3B0ciB8fCBvcHMtPnN0b3JlID09IE5V
TEwpDQorCQlnb3RvIG91dDsNCisJaWYgKHJlcS0+bmV3bGVuID4gUEFHRV9T
SVpFLTIpIHsgLyogTlVMIHRlcm1pbmF0ZWQgKyAnXG4nICovDQorCQllcnJv
ciA9IEVJTlZBTDsNCiAJCWdvdG8gb3V0Ow0KLQllcnJvciA9IFNZU0NUTF9J
TihyZXEsIGJ1ZiwgUEFHRV9TSVpFKTsNCisJfQ0KKwllcnJvciA9IFNZU0NU
TF9JTihyZXEsIGJ1ZiwgcmVxLT5uZXdsZW4pOw0KIAlpZiAoZXJyb3IpDQog
CQlnb3RvIG91dDsNCisJKChjaGFyKilidWYpW3JlcS0+bmV3bGVuXSA9ICdc
bic7DQorCSgoY2hhciopYnVmKVtyZXEtPm5ld2xlbisxXSA9ICdcMCc7DQog
CWxlbiA9IG9wcy0+c3RvcmUoa29iaiwgYXR0ciwgYnVmLCByZXEtPm5ld2xl
bik7DQogCWlmIChsZW4gPCAwKQ0KIAkJZXJyb3IgPSAtbGVuOw0KLS0gDQox
LjguMC4xDQoNCg==
--967339439-20611481-1355377809=:90705--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201212130600.qBD601w5046021>