crossbow discuss - Aug 2008 - [Fwd: [networking-discuss] code-review: fine-grained privileges for datalink administration]

Crossbow team,

The following is of interest to the Crossbow project.  Since a large
chunk of these changes also exist in the Crossbow gate, the delivery of
this wad will result in fewer lines of changes for Crossbow''s delivery.
If someone on Crossbow could participate in this review, that would be a
bonus (Eric Cheng made original changes in the Crossbow gate at some
point last year).

-Seb

-------- Forwarded Message --------
From: Sebastien Roy <Sebastien.Roy at Sun.COM>
To: networking-discuss at opensolaris.org
Subject: [networking-discuss] code-review: fine-grained privileges for
datalink administration
Date: Mon, 04 Aug 2008 11:09:47 -0400

Hi Folks,

I''m looking for code-reviewers for the following set of changes:

PSARC 2008/473 Fine-Grained Privileges for Datalink Administration
6695904 least privileges for datalink actions
6729477 pcwl accidentally requires privileges for WLAN_GET_PARAM ioctl
6679049 ucred_t leak in dlmgmtd

Webrev:

http://cr.opensolaris.org/~seb/dladm-privs-webrev/

Don''t be put-off by the webrev''s claim of 1800+ lines of
changes, as
only 800 or so of those lines are modified or new.  There are over 1000
lines of deleted code.

Workspace with cscope database:

/net/zhadum.east/export/ws/seb/dladm-privs-hg/

The high-level architecture behind the change is described in the PSARC
fast-track case log:

http://www.opensolaris.org/os/community/arc/caselog/2008/473/

One thing to keep in mind while reviewing these changes is that the dld
control device is no longer a STREAMS device, but a regular character
device.  For one, STREAMS serves no purpose here.  The device simply
processes GLDv3 ioctls, and that''s it.  Removing STREAMS from this part
of the framework makes the code simpler and removes the data-size
limitations that the Crossbow project has run into with I_STR.

One additional note is that I haven''t done any install testing yet, and
one issue that I may run into is the presence (or lack thereof) of
the /dev/dld device link at first boot.  I may need to ship a /dev/dld
link as part of the SUNWcsd package to make sure that the link always
exists.

Thanks in advance,
-Seb


_______________________________________________
networking-discuss mailing list
networking-discuss at opensolaris.org

I''ll take a look.
I''m also asking for another pair of eye balls from crossbow team

    Kais.

Sebastien Roy wrote:
> Crossbow team,
>
> The following is of interest to the Crossbow project.  Since a large
> chunk of these changes also exist in the Crossbow gate, the delivery of
> this wad will result in fewer lines of changes for Crossbow''s
delivery.
> If someone on Crossbow could participate in this review, that would be a
> bonus (Eric Cheng made original changes in the Crossbow gate at some
> point last year).
>
> -Seb
>
> -------- Forwarded Message --------
> From: Sebastien Roy <Sebastien.Roy at Sun.COM>
> To: networking-discuss at opensolaris.org
> Subject: [networking-discuss] code-review: fine-grained privileges for
> datalink administration
> Date: Mon, 04 Aug 2008 11:09:47 -0400
>
> Hi Folks,
>
> I''m looking for code-reviewers for the following set of changes:
>
> PSARC 2008/473 Fine-Grained Privileges for Datalink Administration
> 6695904 least privileges for datalink actions
> 6729477 pcwl accidentally requires privileges for WLAN_GET_PARAM ioctl
> 6679049 ucred_t leak in dlmgmtd
>
> Webrev:
>
> http://cr.opensolaris.org/~seb/dladm-privs-webrev/
>
> Don''t be put-off by the webrev''s claim of 1800+ lines of
changes, as
> only 800 or so of those lines are modified or new.  There are over 1000
> lines of deleted code.
>
> Workspace with cscope database:
>
> /net/zhadum.east/export/ws/seb/dladm-privs-hg/
>
> The high-level architecture behind the change is described in the PSARC
> fast-track case log:
>
> http://www.opensolaris.org/os/community/arc/caselog/2008/473/
>
> One thing to keep in mind while reviewing these changes is that the dld
> control device is no longer a STREAMS device, but a regular character
> device.  For one, STREAMS serves no purpose here.  The device simply
> processes GLDv3 ioctls, and that''s it.  Removing STREAMS from this
part
> of the framework makes the code simpler and removes the data-size
> limitations that the Crossbow project has run into with I_STR.
>
> One additional note is that I haven''t done any install testing
yet, and
> one issue that I may run into is the presence (or lack thereof) of
> the /dev/dld device link at first boot.  I may need to ship a /dev/dld
> link as part of the SUNWcsd package to make sure that the link always
> exists.
>
> Thanks in advance,
> -Seb
>
>
> _______________________________________________
> networking-discuss mailing list
> networking-discuss at opensolaris.org
>
> _______________________________________________
> crossbow-discuss mailing list
> crossbow-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/crossbow-discuss
>
>

On Fri, 2008-08-08 at 14:30 -0700, Kais Belgaied wrote:
> I''ll take a look.
> I''m also asking for another pair of eye balls from crossbow team
Thanks very much, Kais.
-Seb

Hi Seb,

the changes look good to me.
only a handful of comments:

kb-01
os/priv-defs line 390 s/PRIV_SYS_DATALINK_CONFIG/PRIV_SYS_DL_CONFIG

kb-02
aggr_ctl.c
103,106,119
how about computing (nports * sizeof (laioc_port_t)) once, save the 
result in a local variable, and use that variable,
as often as you need, instead of computing the same quantity 3 times in 
the same function

kb-03
similar comment about 224,226,243

kb-04
dld_drv.c
525,526. The context is blockable here. KM_SLEEP is fine.

kb-05
1102,1108 are returning without calling ddi_release_devi(dip).

    Kais

On 08/04/08 08:23, Sebastien Roy wrote:
> Crossbow team,
>
> The following is of interest to the Crossbow project.  Since a large
> chunk of these changes also exist in the Crossbow gate, the delivery of
> this wad will result in fewer lines of changes for Crossbow''s
delivery.
> If someone on Crossbow could participate in this review, that would be a
> bonus (Eric Cheng made original changes in the Crossbow gate at some
> point last year).
>
> -Seb
>

Hi Kais,

Thanks for the review.

On Thu, 2008-08-14 at 22:50 -0700, Kais Belgaied wrote:
> kb-01
> os/priv-defs line 390 s/PRIV_SYS_DATALINK_CONFIG/PRIV_SYS_DL_CONFIG
Fixed.
> 
> kb-02
> aggr_ctl.c
> 103,106,119
> how about computing (nports * sizeof (laioc_port_t)) once, save the 
> result in a local variable, and use that variable,
> as often as you need, instead of computing the same quantity 3 times in 
> the same function
Fixed.
> 
> kb-03
> similar comment about 224,226,243
Fixed.
> 
> kb-04
> dld_drv.c
> 525,526. The context is blockable here. KM_SLEEP is fine.
Do you mean 425?  To sleep or not to sleep, that is the question.  While
it''s safe to sleep, do we really want to?  If the system is running low
on memory, do we want such ioctl threads to hang, or to simply return
ENOMEM to the caller so that they can try again later?  My feeling is
that the latter is the more systemically polite thing to do, and I''m
inclined to make such allocations KM_NOSLEEP allocations.  On that note,
I should probably change the allocation in drv_ioctl() to be KM_NOSLEEP
to keep the behavior consistent.  Thoughts?
> 
> kb-05
> 1102,1108 are returning without calling ddi_release_devi(dip).
Indeed, fixed.

The webrevs incorporating all changes since the initial webrev
(including these changes) are here:

against onnv:
http://cr.opensolaris.org/~seb/dladm-privs.2/
against the last webrev:
http://cr.opensolaris.org/~seb/dladm-privs.2.inc/

Thanks,
-Seb

> Do you mean 425?  To sleep or not to sleep, that is the question.  While
 > it''s safe to sleep, do we really want to?  If the system is
running low
 > on memory, do we want such ioctl threads to hang, or to simply return
 > ENOMEM to the caller so that they can try again later?  My feeling is
 > that the latter is the more systemically polite thing to do, and
I''m
 > inclined to make such allocations KM_NOSLEEP allocations.

FWIW, I agree -- just because you can sleep doesn''t mean you should,
especially given that KM_SLEEP allocations are uninterruptible, which
means the poor admin can''t even ^C the hung dladm process.

 > On that note, I should probably change the allocation in drv_ioctl() to
 > be KM_NOSLEEP to keep the behavior consistent.  Thoughts?

Sounds good.

--
meem

On 08/15/08 14:35, Sebastien Roy wrote:
>
>> kb-04
>> dld_drv.c
>> 525,526. The context is blockable here. KM_SLEEP is fine.
>>     
>
> Do you mean 425?  To sleep or not to sleep, that is the question.  While
> it''s safe to sleep, do we really want to?  If the system is
running low
> on memory, do we want such ioctl threads to hang, or to simply return
> ENOMEM to the caller so that they can try again later?  My feeling is
> that the latter is the more systemically polite thing to do, and
I''m
> inclined to make such allocations KM_NOSLEEP allocations.  On that note,
> I should probably change the allocation in drv_ioctl() to be KM_NOSLEEP
> to keep the behavior consistent.  Thoughts?
>   
OK. KM_NOSPEEP is reasonable as a defensive measure.

    Kais.

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/crossbow-discuss/attachments/20080815/208bf1c0/attachment.html>