From nobody Sat Oct 12 21:09:07 2024 X-Original-To: freebsd-hackers@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4XQx212RSqz5Z02B for ; Sat, 12 Oct 2024 21:09:13 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-qt1-x82e.google.com (mail-qt1-x82e.google.com [IPv6:2607:f8b0:4864:20::82e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "WR4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4XQx210WQDz4VQ6 for ; Sat, 12 Oct 2024 21:09:13 +0000 (UTC) (envelope-from markjdb@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-qt1-x82e.google.com with SMTP id d75a77b69052e-45effbc3348so26564781cf.2 for ; Sat, 12 Oct 2024 14:09:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1728767352; x=1729372152; darn=freebsd.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:sender :from:to:cc:subject:date:message-id:reply-to; bh=z4M6zJH6MY2KJ6Fcq5UE1s62wpDfMV7OtOrL/K7uees=; b=WrYJ5ntxBh9aLKif5GcFeq9If7EPwtvE6vbXqxARSDlx23d15ui6HI7eN2omK3UiM+ NoK3EchSM82vaAP8FAxiN1VAZp/1kbPIelXYEcMKUNORdKUUPjn/2T1m4JluSjxXl8qi oIiCjmfQGqnPV1Ip7AIkNXGVKFuOeMraXIpAKZrwdfK5omyvYwFjoAjAXCg5JU6KnkUg 44zCxVEnwJRlYW4T2DaWOe19QpsEdB5CHjlEimMxrKnepozGDqn/7Yj0T9aauKnhz95Z 2MRYjY/r4F4IzFGw099h6qtgRt8ntdUKWUquJuOKdUvijpMS5ObyRyewgXBIZq0MLrst Vi/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728767352; x=1729372152; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:sender :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=z4M6zJH6MY2KJ6Fcq5UE1s62wpDfMV7OtOrL/K7uees=; b=Lr/kyuL4OVdKLmzgc99RysCycD5IEbXLiTdKRa7tIucszvpaoWMkXN5kqcK8sWhY2O LJ6WHl97wbtrmtM+yF8I4zw5yYKmVcztuIojx5swK9qaY2jO29PSOtKHr8MnVx6h+q63 r4hMiB3Tvr/WjXip3GFcNE4C0sG/QhKW7mg02bpe0h1sxCDqCfbPgaEhJfuQXjRzBduw RxaK1RqQc3yrK43Y12n170hutTV0qAUUYqHh6fdfoeafmLyZOWNyL7gG3feyVSwgVNae 1WpsO/0JvrWb6UvsiQY9HQq1BCTXMchyGBiO8vn14jXW1EMA8klFjsHAv1rgShZMEVML i+/Q== X-Forwarded-Encrypted: i=1; AJvYcCVn1s/llzxwDpToqlHORbTT0jfqkHKMAIAxLJzhGKxdrUYhi+guYwFf4z1syi06PkspsyAaUUT90F/wNjs+Tto=@freebsd.org X-Gm-Message-State: AOJu0Yy79y/F1YCyNEJWBM79SaL03E3tSoivpZ7/dAN6PDRw5/xH0P5Z GQtUt+nOvmueCQKTGMQ+ydoWABO3SRVO07lcFa2eA08Kataq0b6+NOGd3w== X-Google-Smtp-Source: AGHT+IFNTev1NJ/EdN4jbcexct2nwxyVCOP/0M0R1Wh+ndOx6LFhN46esxiZTDx9eGAKIsXF4/qybw== X-Received: by 2002:a05:622a:40cd:b0:45f:106:590e with SMTP id d75a77b69052e-460584221d8mr68150111cf.24.1728767352092; Sat, 12 Oct 2024 14:09:12 -0700 (PDT) Received: from nuc (192-0-220-237.cpe.teksavvy.com. [192.0.220.237]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-46042894249sm28100021cf.86.2024.10.12.14.09.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 12 Oct 2024 14:09:10 -0700 (PDT) Date: Sat, 12 Oct 2024 17:09:07 -0400 From: Mark Johnston To: "David E. Cross" Cc: Cy Schubert , Enji Cooper , FreeBSD Hackers Subject: Re: Review D38047 ... and then there was one.... Message-ID: References: <20241007150830.939DC334@slippy.cwsent.com> <20241011153526.431EA110@slippy.cwsent.com> <2b399f81-ccca-0eb5-3175-5b59e4253b7b@crossfamilyweb.com> List-Id: Technical discussions relating to FreeBSD List-Archive: https://lists.freebsd.org/archives/freebsd-hackers List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-hackers@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US] X-Rspamd-Queue-Id: 4XQx210WQDz4VQ6 X-Spamd-Bar: ---- On Sat, Oct 12, 2024 at 01:22:57PM -0400, David E. Cross wrote: > > On 10/12/24 11:46, Mark Johnston wrote: > > And if all of the commits are pushed together, which they would be, then > > users will only see the end result anyway, so there is nothing > > inherently dangerous about splitting the change into easier-to-review > > commits. > > > > Even if the whole patch is applied atomically, it affects multiple > > binaries in the system (at least libc.so and nscd), and there's nothing > > guaranteeing that the update is somehow applied atomically across the > > whole system. What if some applications using an older libc end up > > still running with an updated nscd? That's an untested configuration, > > but it might still arise if the whole patch is committed as one unit. > > > > I would split it up like this: > > 1. Rename the cycle prevent symbol. This is a cosmetic change which is > > irrelevant to the correctness of the rest of the patch. > > 2. In nscd, fix the problem with negative lookups. I can't easily see > > if anything other than the query.c change is required there, which is > > part of the reason I asked for the patch to be split up. > > 3. Use thread-safe lookups in nscd's passwd.c and group.c. For the > > latter, a bit of work is needed to tease out the bits that add > > getgrouplist() handling. I believe that consists of > > wrap_getgrouplist() and the handling of nss_lt_pivot in > > group_lookup_func(). > > 4. Add getgrouplist() support to nscd. At this point, libc doesn't > > dispatch getgrouplist() to nscd, so this change has no effect. > > 5. Teach libc to marshall and unmarshall getgrouplist() requests. This > > could reasonably be combined with 4 since the logic is coupled. > > 6. Fix nscd to export the cycle prevention symbol. > > > > Just the exercise of reading through the patch again with an eye to > > splitting it up helped me understand it better and find a couple of > > buglets. It can be split up mechanically using git add -p for the most > > part (aside from the fact that parts 3 and 4 touch nscd's group.c). > > > > From my point of view, your patch hasn't gotten a lot of attention > > because > > 1. It's big and affects multiple binaries. > > 2. There are few committers familiar with our NSS implementation. > > > > The point of splitting it is to help with 1. It requires more work from > > developers, but then reviewers are much more likely to read and actually > > understand it, which helps with 2 in the long run. > > > Ok, if this is just part of the review, and it all goes in at once my > concerns are ameliorated.  I was afraid this was going to get piecemeal > applied and then the bulk of the work, and the only real reason I did this > was going to sit on a shelf; that is not terribly motivating to do a bunch > of *additional* work I will be sure to push all of the commits at once. :) > To address your points individually. > > 1. yes, and I see you correctly place the final step at 6 for this. > > 2. Of note on this fix, this is actually only required once the symbol is > exported, it has no real effect prior to this, (you can see above at line > 755 this is gated by the perform actual lookup check (which won't even work > now).  So that is to say this doesn't break things any more than they are > already broken (ie, not high priority) > > 3. This I do NOT believe is required.  I spent a lot of time going over the > threading here; the various getpw and getgr entries already use thread local > storage, so within a thread you are safe as long as you preserve values > across calls as needed (and each thread within nscd always works to > completion, even the lt_all ones).  I remember starting a bigger refactor to > address that (since it was implemented inconsistently across different > types). But I decided I wanted no part of that change.   Or are you > referring to my code here; upon re-reading I am not sure if you are talking > about pulling my agent code here, or if you think more thread hardening is > required. I'm referring to your changes to passwd.c and group.c (excluding getgrouplist() bits) in nscd. It's been a while since I looked at how the nsswitch code manages thread-local storage, so I don't have an opinion yet on whether the change is actually required. If you don't think it is, then it'd be easier to drop that change for now. > 4/5: agreed > > 6: agreed as above. > > If you could point me at the buglets I will work on addressing them (or > discussing them, since I believe I am thread safe here), and I will also > split this up. I reported them in some comments on https://reviews.freebsd.org/D38047 > Additionally I plan to tackle the other bug raised in -hackers about > 'getent' ALWAYS hitting ALL databses.. but I need to wrap my head around > that more; it has been multiple years since I have been in this code and > dusting off the cobwebs is taking time (another reason I wasn't big on > splitting this up, I hadn't touched it in forever). > > > How do you recommend I post this, are these individual PRs on > reviews.freebsd.org; do I post the diff differently? Individual PRs on reviews.freebsd.org is fine. We also use github pull requests these days, which may be easier for you. Either is ok with me. To submit a series of git commits through phabricator, you can try installing the freebsd-git-devtools package. With that, given a range of commits .., you can upload them to phabricator with a single command: $ git arc -r markj ~.. (This will add me as a reviewer.) "git arc --help" will give more complete usage instructions. It's not necessary to use this tool to upload patches, I just find it easier.