From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server-vie001.gnuweeb.org X-Spam-Level: X-Spam-Status: No, score=-1.2 required=5.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,URIBL_DBL_BLOCKED_OPENDNS, URIBL_ZEN_BLOCKED_OPENDNS autolearn=ham autolearn_force=no version=3.4.6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gnuweeb.org; s=new2025; t=1757133198; bh=yOtbnVomhdqf3f7JSO6JC/IbH+Z/gw6IceuAKbMY9Cc=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type:Content-Transfer-Encoding:Message-ID:Date:From: Reply-To:Subject:To:Cc:In-Reply-To:References:Resent-Date: Resent-From:Resent-To:Resent-Cc:User-Agent:Content-Type: Content-Transfer-Encoding; b=ZDLAcEP0WFIpax2lfl8+ZL1QjlGKQ0SvHIDvQscqbOKUJ204t7JM2djFH6Q1j1Tk+ Xx83OXP7BZ9pmWdZyh6qXWWrzDXdhoXoho/IqYf4PD6v2UbHZRNFUgXBd+a8wmaGXR LmHrheOrj5XmIxKJ+QBIL9EsI5eBo8GYDDmK+efLtPfv+rQOsnRj9CGO1Q9jL9VNC8 GaOabpaAddp4JGE5RF0D4BGuLePkPoj5+NGZS+gi2rkftpVbpG2LqvcwSOf/yPZk0Q M/PpvRM74261iRwFIwKcQJp1B6dlEGgR4ncVTRCHL+puM696AvX/wCNuOaqQkEIr6R ifJcBwFEEGu0Q== Received: from mail-lj1-f172.google.com (mail-lj1-f172.google.com [209.85.208.172]) by server-vie001.gnuweeb.org (Postfix) with ESMTPSA id 3EB1231279B2 for ; Sat, 6 Sep 2025 04:33:18 +0000 (UTC) Received: by mail-lj1-f172.google.com with SMTP id 38308e7fff4ca-336cee72f40so24689071fa.2 for ; Fri, 05 Sep 2025 21:33:18 -0700 (PDT) X-Forwarded-Encrypted: i=1; AJvYcCWm6ONem6bc2UWvvoe5jDpN6sS21z+OpeAfX6SGtIRRt5uidFJzbY1p5BtTPcJ7MuEcq14g@vger.gnuweeb.org X-Gm-Message-State: AOJu0YwgBNDxNhGz52YJLiw3B6cHXoNw6TMU+fJp3BfiMaS9YZIvOqy3 s0jfVb9GPL++bJ2jsW2umTXCF8FBHcQ8fRh0yo8KoG5hTcTO50pISWZBrzQQrnvQjJrD/U5qy/2 49vya734qZuIFSBifGUil5/BOgEsR4Bg= X-Google-Smtp-Source: AGHT+IFuIvlnSvH0AjLquWU6sc9igVA3n2mMKcm/k4zrqWarYr+uFyCvh305edn8QtDEz4f9t+j33pAZUbmIa4A7ons= X-Received: by 2002:a05:6512:3d0b:b0:560:827f:9ff3 with SMTP id 2adb3069b0e04-56261bc58f5mr314595e87.36.1757133197558; Fri, 05 Sep 2025 21:33:17 -0700 (PDT) MIME-Version: 1.0 References: <20250829075557.598176-1-reyuki@gnuweeb.org> <20250829075557.598176-3-reyuki@gnuweeb.org> In-Reply-To: From: Ahmad Gani Date: Sat, 6 Sep 2025 11:32:41 +0700 X-Gmail-Original-Message-ID: X-Gm-Features: Ac12FXxDvQt_vCm_t1HwdoGHXh1xv0qrITLPhbnTzmKRs27Qe-ki47mBuUQPLhw Message-ID: Subject: Re: [PATCH gwproxy v8 2/2] gwproxy: refactor code base to add experimental raw DNS backend To: Alviro Iskandar Setiawan Cc: Ammar Faizi , "GNU/Weeb Mailing List" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable List-Id: On Fri, Sep 5, 2025 at 11:26=E2=80=AFPM Alviro Iskandar Setiawan wrote: > Having #ifdef CONFIG_RAW_DNS all over the place is horrible. Please do > something else to reduce them. CTRL + F, "def CONFIG_RAW_DNS", 24 > matches. Hi Sir Al, First I want to say thank you for reviewing my patchset (and sir Ammar for pinging it). I thought it was necessary to mark that a certain block belongs to the experimental raw DNS feature. If I reduce the use of these marks, it might introduce unnecessary code into the compiled version when this experimental feature is disabled. > On Fri, Aug 29, 2025 at 2:56 PM Ahmad Gani wrote: >> +void cp_nsaddr(struct gwp_dns_ctx *ctx, struct gwp_sockaddr *addr, uint= 8_t *addrlen) >> +{ >> + *addr =3D ctx->ns_addr; >> + *addrlen =3D ctx->ns_addrlen; >> +} > > Why is this exposed globally? I don't think it's worth a function > call. Better be inlined. I can't access members of the gwp_dns_ctx struct directly from another file, as it seems to be treated as an opaque pointer outside of dns.c. If I make it an inline function, it would only be accessible within dns.c. Should I move the full declaration of the struct to the header file? >> +int gwp_dns_process(struct gwp_dns_ctx *ctx, struct gwp_dns_entry *e) >> +{ >> + struct gwdns_addrinfo_node *ai; >> + uint8_t buff[UDP_MSG_LIMIT]; >> + ssize_t r; >> + >> + r =3D __sys_recvfrom( >> + e->udp_fd, buff, sizeof(buff), 0, >> + &ctx->ns_addr.sa, (socklen_t *)&ctx->ns_addrlen >> + ); >> + if (r <=3D 0) >> + return (int)r; > > The subsequent code calls gwdns_parse_query(), and you noted that a > return value of 0 indicates success, and by the end of > gwp_dns_process(), it returns what was returned by > gwdns_parse_query(). That's it. It means, if successful, it returns 0. > What happens if __sys_recvfrom() returns 0? Doesn't that mean it's > also considered successful? If so, why? Yeah, that's an error on my part. I will set r to -EINVAL for lengths in the range 0-38. Since there is no short send in UDP, there's no way it can be a valid standard query if the length is less than 38 bytes: - 12 bytes for the header section - 10 bytes for the query section - 16 bytes for the answer section The shortest valid domain length is 3 characters: 1 for the second-level label and 2 for the TLD label. You can try `dig +short +noedns g.cn` and observe it in Wireshark. So far, the shortest standard query response I've found is 38 bytes. >> + if (cfg->use_raw_dns) { >> +#ifdef CONFIG_RAW_DNS >> + r =3D convert_str_to_ssaddr(cfg->ns_addr_str, &ctx->ns_addr, 53); >> + if (r) >> goto out_destroy_mutex; >> + ctx->ns_addrlen =3D ctx->ns_addr.sa.sa_family =3D=3D AF_INET >> + ? sizeof(ctx->ns_addr.i4) >> + : sizeof(ctx->ns_addr.i6); >> + ctx->entry_cap =3D DEFAULT_ENTRIES_CAP; >> + ctx->entries =3D malloc(ctx->entry_cap * sizeof(*ctx->entries)); >> + if (!ctx->entries) >> + goto out_destroy_mutex; >> +#endif >> + } else { > > Bad indentation. Apart from bad indentation, if the malloc() call > fails, r still contains 0, and you goto out_destroy_mutex, returning 0 > on failure. Shouldn't you set -ENOMEM instead? You're right to point that out, I'll fix it. >> + txid =3D (uint16_t)rand(); > > Why do you use a random number for the txid? I don't think rand() > guarantees uniqueness. Also, I don't find the srand() call anywhere. > That's equivalent to having srand(1). > > What about using a simple incrementing counter instead (which > guarantees the uniqueness)? > It will indeed reset after 65535, but that should give enough room to > complete prior DNS requests first. With a simple counter, the reuse of > an ID will always be 65535 queries away. Without calling srand, it still generates random numbers, so I thought it was fine. But I agree with your suggestion=E2=80=94an incrementing counter = is better, because randomness doesn=E2=80=99t guarantee uniqueness and is more likely to cause collisions than a counter. >> struct gwp_dns_entry { >> +#ifdef CONFIG_RAW_DNS >> + uint32_t idx; >> + int udp_fd; > > AFAIU, when using the raw DNS feature you bring. ev_fd is not used, > instead you use udp_fd. So I think it is not worth adding extra unused > space. Better be an anonymous union if the name needs to be different. > No? That's indeed a more efficient way to handle it. Let's go with that idea. >> +#ifdef CONFIG_RAW_DNS >> +static int arm_poll_for_raw_dns_query(struct gwp_wrk *w, >> + struct gwp_conn_pair *gcp) >> +{ >> + struct gwp_dns_entry *gde =3D gcp->gde; >> + struct gwp_dns_ctx *dctx; >> + struct gwp_sockaddr addr; >> + uint8_t addrlen; >> + ssize_t r; >> + >> + dctx =3D w->ctx->dns; >> + cp_nsaddr(dctx, &addr, &addrlen); >> + r =3D __sys_sendto( >> + gde->udp_fd, gde->payload, gde->payloadlen, MSG_NOSIGNAL, >> + &addr.sa, addrlen >> + ); >> + >> + return (int)r; >> +} >> +#endif > > Why is this function named arm_poll_for_raw_dns_query()? There is no > poll-arming here. You're simply calling __sys_sendto(). Please write a > function name based on what it does. It's confusing. Lol, I'm surprised myself to see that function name. I'm not sure what I was thinking when I wrote it=E2=80=94sorry for the confusion. > Bad indentation. Why are there many of those? Sorry about the indentation issues. They often happen when I add a new if block and forget to adjust the indentation afterward, I'll fix it. -- Ahmad Gani